xfs
[Top] [All Lists]

Re: odd code in xfs_remove

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: odd code in xfs_remove
From: David Chinner <dgc@xxxxxxx>
Date: Fri, 11 Apr 2008 12:31:45 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080410185445.GA7521@lst.de>
References: <20080410185445.GA7521@lst.de>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
On Thu, Apr 10, 2008 at 08:54:45PM +0200, Christoph Hellwig wrote:
> unlike xfs_rmdir or xfs_rename xfs_remove always passes 0 instead of the
> reserved blocks to xfs_dir_removename which means the latter always
> operated in the ENOSPC mode.  This seems rather strange to me, so a
> second pair of eyes, especially on a tree with history back to day 0
> would be nice.

Originially, back in the days of dir1 (1994) when xfs_dir_removename()
was expanded to take a block count, only xfs_rename() passed a value
in (MAX_EXT_NEEDED). Both xfs_rmdir and xfs_remove passed zero.

Then some time later (pv 519072):

irix/kern/fs/xfs/xfs_vnodeops.c - 1.356
        - Add more vnode tracing points.  Remove some cruft from merge errors.
          Speed up xfs_remove by getting a real space reservation unless the fs
          is full, then reverting to a zero space reservation (bug 519072). 

The speed up was for dir v1, which doesn't exist on linux. Then in pv 653987:

irix/kern/fs/xfs/xfs_vnodeops.c - 1.395
        - In xfs_link, call xfs_dir_canenter before xfs_dir_createname if we
          have no space reservation.
          In xfs_rmdir, get a 0 space reservation if we can't get the real space
          reservation (as is done for xfs_remove).
          In xfs_symlink, properly account for resblks throughout so we don't 
          make it go negative to a xfs_bmapi call.

The key point here is that in rev 1.395, the variable resblks got passed
into xfs_dir_removename(), whilst in 1.356, it didn't. This was work
that preceeded dir v2, so was probably fixing bugs found as a result.

The remove name case with resblks = 0 avoids block allocation during
modification of the alloc btree to insert newly freed blocks at
ENOSPC; instead it just leaves the empty leaf block in place in the
directory.

Looks like no harm is done by this and it's a rarely hit corner
case, but it would appear that we should be passing in resblks in
remove if only to avoid a potential transaction reservation
overrun....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group


<Prev in Thread] Current Thread [Next in Thread>