Re: [PATCH 10/25] xfs: factor extent allocation out of xfs_bmapi

[Alex Elder <aelder@sgi.com>]

On Wed, 2011-08-24 at 02:04 -0400, Christoph Hellwig wrote:
> To further improve the readability of xfs_bmapi(), factor the extent
> allocation out into a separate function. This removes a large block
> of logic from the xfs_bmapi() code loop and makes it easier to see
> the operational logic flow for xfs_bmapi().
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

OK, this looks very good.  I have a spot that I chased
for a while to verify it produced the same functionality
as before, but I just gave up because it was just taking
much too much time.  I'll point it out below, just for
the record, but I'm not too concerned about it.  Everything
else looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>

> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================

. . .

> @@ -4750,144 +4893,31 @@ xfs_bmapi(
>  		 * that we found, if any.
>  		 */
>  		if (wr && (inhole || wasdelay)) {
> -			/*
> -			 * For the wasdelay case, we could also just
> -			 * allocate the stuff asked for in this bmap call

. . .

> -						ip, whichfork);
> -					cur->bc_private.b.firstblock =
> -						*firstblock;
> -					cur->bc_private.b.flist = flist;
> +			bma.eof = eof;
> +			bma.conv = !!(flags & XFS_BMAPI_CONVERT);
> +			bma.wasdel = wasdelay;
> +			bma.alen = len;
> +			bma.off = bno;
> +			bma.minleft = minleft;
> +
> +			error = xfs_bmapi_allocate(&bma, &lastx, &cur,
> +					firstblock, flist, flags, &nallocs,
> +					&tmp_logflags);

> +			if (error == ENOSPC || error == EDQUOT) {
> +				if (n == 0) {
> +					*nmap = 0;
> +					ASSERT(cur == NULL);
> +					return error;
>  				}

Here is the spot I mentioned above.  I was trying to find out
the circumstances under which ENOSPC or EDQUOT could get returned
by xfs_bmapi_allocate() in order to confirm that this in fact
produces the same effect as before.

I also had a little trouble because there were spots--such
as calling xfs_bmap_isaeof()--that are now encapsulated within
xfs_bmapi_allocate() that previously jumped to error0, but now
will produce an error return from that function.  So now this
doesn't execute the code at error0 in this case.  I didn't
work through it but I trust that the code there would end
up being a series of no-ops anyway.



> -				/*
> -				 * Bump the number of extents we've allocated
> -				 * in this call.
> -				 */
> -				nallocs++;
> -			}

. . . .

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs