Re: [LTP/VFS] fcntl SETLEASE fails on ramfs/tmpfs

david m. richter
2008-04-30T18:15:14Z

On Wed, 30 Apr 2008, J. Bruce Fields wrote:

> On Tue, Apr 29, 2008 at 07:21:02PM -0400, david m. richter wrote:
> > On Tue, 29 Apr 2008, J. Bruce Fields wrote:
> >
> > > On Tue, Apr 29, 2008 at 01:54:54PM -0700, Andrew Morton wrote:
> > > > On Tue, 29 Apr 2008 11:42:48 +0800
> > > > "Bryan Wu" <cooloney@kernel.org> wrote:
> > > >
> > > > > Hi folk,
> > > > >
> > > > > This days I am digging into this LTP bug reported on our Blackfin test
> > > > > machine, but I think it is general for other system.
> > > > > https://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_id=141&tracker_item_id=3743
> > > > >
> > > > > And I also found Kumar Gala reported this similar bug before.
> > > > > http://lkml.org/lkml/2007/11/14/388
> > > > >
> > > > > 1, when opening and creating a new on ramfs/tmpfs, the dentry->d_count
> > > > > will be added one as below:
> > > > > --
> > > > > ramfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
> > > > > {
> > > > > |_______struct inode * inode = ramfs_get_inode(dir->i_sb, mode, dev);
> > > > > |_______int error = -ENOSPC;
> > > > >
> > > > > |_______if (inode) {
> > > > > |_______|_______if (dir->i_mode & S_ISGID) {
> > > > > |_______|_______|_______inode->i_gid = dir->i_gid;
> > > > > |_______|_______|_______if (S_ISDIR(mode))
> > > > > |_______|_______|_______|_______inode->i_mode |= S_ISGID;
> > > > > |_______|_______}
> > > > > |_______|_______d_instantiate(dentry, inode);
> > > > > |_______|_______dget(dentry);|__/* Extra count - pin the dentry in core */
> > > > > |_______|_______error = 0;
> > > > > |_______|_______dir->i_mtime = dir->i_ctime = CURRENT_TIME;
> > > > > |_______}
> > > > > |_______return error;
> > > > > }
> > > > > --
> > > > > The dget(dentry) call introduces an extra count, why?
> > > > > it is the same in tmpfs.
> > > >
> > > > Because those dentries have no backing store. Their sole existance is in
> > > > the dentry cache which is normally reclaimable. But we can't reclaim these
> > > > dentries because there is nowhere from where they can be reestablished.
> > > >
> > > > > 2, when calling fcntl(fd, F_SETLEASE,F_WRLCK), it will return -EAGAIN
> > > > > --
> > > > > |_______if ((arg == F_WRLCK)
> > > > > |_______ && ((atomic_read(&dentry->d_count) > 1)
> > > > > |_______|_______|| (atomic_read(&inode->i_count) > 1)))
> > > > > |_______|_______goto out;
> > > > > --
> > > >
> > > > Sucky heuristic.
> > >
> > > Yes.
> > >
> > > >
> > > > > because the dentry->d_count will be 2 not 1. I tested ext2 on Blackfin, it is 1.
> > > > >
> > > > > 3, so I guess maybe the dget(dentry) of ramfs_mknod is useless. But
> > > > > after remove this dget(),
> > > > > the ramfs can not be mounted as rootfs at all.
> > > >
> > > > Interesting. Presumably it got reclaimed synchronously somehow.
> > > >
> > > > > Is the bug in generic_setlease() or in the ramfs/tmpfs inode create function?
> > > > >
> > > > > Of course, simply remove the test '((atomic_read(&dentry->d_count) >
> > > > > 1)' can workaround this issue.
> > > >
> > > > I guess we should make the generic_setlease() heuristic smarter.
> > > >
> > > > Of course the _reason_ for that heuristic is uncommented and lost in time.
> > > > And one wonders what locking prevents it from being totally racy, and if
> > > > "none", what happens when the race hits. Sigh.
> >
> > i'm not sure which particular kernel version we're talking about,
> > but i think the intent was to rely on the BKL. i noticed a couple cases
> > where this didn't actually hold -- e.g., bruce has a patch queued to move
> > the kmalloc in generic_setlease() so that it precedes the
> > d_count/i_writecount checks and covers the race he mentions below. an
> > earlier patch closed a similar thing with get_write_access() when handling
> > a truncate, etc.
> >
> > fyi (well, not you, bruce): relatedly, i have a set of patches to
> > introduce per-inode lease enabling/disabling (so we can fully implement
> > NFSv4.0 file delegations and v4.1 directory delegations) which expand the
> > cases where leases are broken (e.g. unlink) and which make it somewhat
> > more explicit when it's safe to lease/break. perhaps the next merge
> > window for the first set of them.
> >
> >
> > > Yes, I think the race is:
> > >
> > > 1. generic_setlease(., F_WRLCK, .) checks d_count and i_count,
> > > both are 1.
> > >
> > > 2. a read open comes in, calls break_lease which finds no lease
> > > and continues happily on.
> > >
> > > 3. generic_setlease() sets the write lease.
> > >
> > > The most likely consequences are that a local reader gets out-of-date
> > > data for a file that a Samba client has modified.
> > >
> > > I suppose that re-checking the d_count and i_count after step 3 might
> > > close the race.
> >
> > as things currently stand, i believe that race can only happen if
> > the leaser is blocking on the kmalloc.
>
> Neither break_lease() (the shortcut inlined function, not the full
> __break_lease()) nor any of the open code that I can see is under the
> BKL, so unless I'm missing something, that code is racy.
>
> --b.

sorry, obviously you're right; if ->i_flock is null, break_lease()
is doing the bad thing. i've been through so many reworkings of that code
that i'd forgotten which race was where; i came into this thread midway
and should've reacquainted myself with the actual code in question. my
bad :-/

> > but yeah, the d_count check is
> > pretty frail anyway ...
> >
> >
> > > > I suppose a stupid fix would be to set (and later clear) a new flag in
> > > > dentry.d_flags which means
> > > >
> > > > this dentry is pinned by a ram-backed device, so d_count==2 means
> > > > "unused""
> > > >
> > > > But it would be better to work out exactly what generic_setlease() is
> > > > trying to do there, and do it in a better way.
> > >
> > > Yes. What it's supposed to do is provide exclusion between opens and
> > > write leases.
> > >
> > > We already have a mechanism that provides exclusion between write opens
> > > and exec, using the i_writecount, so we're using that for read leases.
> > > I suppose it'd be possible to do something similar for write leases;
> > > would there be smp scalability problems associated with counting all the
> > > read opens of a given inode? Other problems?

interesting; is this feasible? i'd like to hear what folks think.

> > >
> > > Even with this problem solved, I'm not convinced write leases are very
> > > useful as implemented. Their only current user is Samba, which uses
> > > them to grant exclusive access to given files to allow clients to cache
> > > writes.
> > >
> > > Samba knows when to revoke that exclusive access because the lease
> > > subsystem signals it on a read open of the file. It doesn't revoke on
> > > stat, however. This causes problem. E.g., say Samba takes out a lease
> > > and tells some client it can now cache its writes indefinitely.
> > > Meanwhile a local application (say, make) is polling that file for
> > > changes using stat. They never see those changes.
> > >
> > > The NFSv2/v3 server for some reason has its own one-off hack that
> > > reports the ctime as now for on any write-leased file, which leads
> > > people to complain about spurious rebuilds:
> > >
> > > http://bugzilla.kernel.org/show_bug.cgi?id=9454
> > >
> > > The one thing I suspect is *not* a really serious problem here is the
> > > reported LTP failure, since probably the only user of this is Samba,
> > > which probably doesn't do a lot of tmpfs exports, and in any case it can
> > > probably soldier on (if with degraded performance--how badly I don't
> > > know) without getting the write lease it wants.
> > >
> > > --b.
> > >
>