[PATCH 2/2] xfs: fix getbmap vs mmap deadlock

[Felix Blyakher]

On Feb 24, 2009, at 7:39 AM, Christoph Hellwig wrote:

> xfs_getbmap (or rather the formatters called by it) copy out the  
> getbmap
> structures under the ilock, which can deadlock against mmap.  This has
> been reported via bugzilla a while ago (#717) and has recently also
> shown up via lockdep.
>
> So allocate a temporary buffer to format the kernel getbmap structures
> into and then copy them out after dropping the locks.
>
> A little problem with this is that we limit the number of extents we
> can copy out by the maximum allocation size,

Actually with the patch we either get all requested extents, or none
if we fail to get memory for them.
Should we teach the callers to expect ENOMEM and repeat the call
to xfs_getbmap with smaller number of extents?

Felix

> but I see no real way
> around that.

>
>
>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
>
> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c	2009-02-23 20:38:27.512925014 +0100
> +++ xfs/fs/xfs/xfs_bmap.c	2009-02-23 20:40:46.720926193 +0100
> @@ -5867,12 +5867,13 @@ xfs_getbmap(
> 	int			nexleft;	/* # of user extents left */
> 	int			subnex;		/* # of bmapi's can do */
> 	int			nmap;		/* number of map entries */
> -	struct getbmapx		out;		/* output structure */
> +	struct getbmapx		*out;		/* output structure */
> 	int			whichfork;	/* data or attr fork */
> 	int			prealloced;	/* this is a file with
> 						 * preallocated data space */
> 	int			iflags;		/* interface flags */
> 	int			bmapi_flags;	/* flags for xfs_bmapi */
> +	int			cur_ext = 0;
>
> 	mp = ip->i_mount;
> 	iflags = bmv->bmv_iflags;
> @@ -5948,6 +5949,13 @@ xfs_getbmap(
> 		return XFS_ERROR(EINVAL);
> 	bmvend = bmv->bmv_offset + bmv->bmv_length;
>
> +
> +	if (bmv->bmv_count > ULONG_MAX / sizeof(struct getbmapx))
> +		return XFS_ERROR(ENOMEM);
> +	out = kmem_zalloc(bmv->bmv_count * sizeof(struct getbmapx),  
> KM_MAYFAIL);
> +	if (!out)
> +		return XFS_ERROR(ENOMEM);
> +
> 	xfs_ilock(ip, XFS_IOLOCK_SHARED);
> 	if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
> 		if (ip->i_delayed_blks || ip->i_size > ip->i_d.di_size) {
> @@ -6001,39 +6009,39 @@ xfs_getbmap(
> 		ASSERT(nmap <= subnex);
>
> 		for (i = 0; i < nmap && nexleft && bmv->bmv_length; i++) {
> -			int full = 0;	/* user array is full */
> -
> -			out.bmv_oflags = 0;
> +			out[cur_ext].bmv_oflags = 0;
> 			if (map[i].br_state == XFS_EXT_UNWRITTEN)
> -				out.bmv_oflags |= BMV_OF_PREALLOC;
> +				out[cur_ext].bmv_oflags |= BMV_OF_PREALLOC;
> 			else if (map[i].br_startblock == DELAYSTARTBLOCK)
> -				out.bmv_oflags |= BMV_OF_DELALLOC;
> -			out.bmv_offset = XFS_FSB_TO_BB(mp, map[i].br_startoff);
> -			out.bmv_length = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
> -			out.bmv_unused1 = out.bmv_unused2 = 0;
> +				out[cur_ext].bmv_oflags |= BMV_OF_DELALLOC;
> +			out[cur_ext].bmv_offset =
> +				XFS_FSB_TO_BB(mp, map[i].br_startoff);
> +			out[cur_ext].bmv_length =
> +				XFS_FSB_TO_BB(mp, map[i].br_blockcount);
> +			out[cur_ext].bmv_unused1 = 0;
> +			out[cur_ext].bmv_unused2 = 0;
> 			ASSERT(((iflags & BMV_IF_DELALLOC) != 0) ||
> 			      (map[i].br_startblock != DELAYSTARTBLOCK));
>                         if (map[i].br_startblock == HOLESTARTBLOCK &&
> 			    whichfork == XFS_ATTR_FORK) {
> 				/* came to the end of attribute fork */
> -				out.bmv_oflags |= BMV_OF_LAST;
> +				out[cur_ext].bmv_oflags |= BMV_OF_LAST;
> 				goto out_free_map;
> 			}
>
> -			if (!xfs_getbmapx_fix_eof_hole(ip, &out, prealloced,
> -					bmvend, map[i].br_startblock))
> +			if (!xfs_getbmapx_fix_eof_hole(ip, &out[cur_ext],
> +					prealloced, bmvend,
> +					map[i].br_startblock))
> 				goto out_free_map;
>
> -			/* format results & advance arg */
> -			error = formatter(&arg, &out, &full);
> -			if (error || full)
> -				goto out_free_map;
> 			nexleft--;
> 			bmv->bmv_offset =
> -				out.bmv_offset + out.bmv_length;
> +				out[cur_ext].bmv_offset +
> +				out[cur_ext].bmv_length;
> 			bmv->bmv_length =
> 				max_t(__int64_t, 0, bmvend - bmv->bmv_offset);
> 			bmv->bmv_entries++;
> +			cur_ext++;
> 		}
> 	} while (nmap && nexleft && bmv->bmv_length);
>
> @@ -6043,6 +6051,16 @@ xfs_getbmap(
> 	xfs_iunlock_map_shared(ip, lock);
>  out_unlock_iolock:
> 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> +
> +	for (i = 0; i < cur_ext; i++) {
> +		int full = 0;	/* user array is full */
> +
> +		/* format results & advance arg */
> +		error = formatter(&arg, &out[i], &full);
> +		if (error || full)
> +			break;
> +	}
> +
> 	return error;
> }
>
>
> _______________________________________________
> xfs mailing list
> xfs at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs