Considering remove spinlock around f_op->setlease()

Considering remove spinlock around f_op->setlease()

[Feng Shuo]

Hi guys,

Since 2.6.37 (commit 72f98e72), f_op->setlease() is called with a
spinlock held (file_lock_lock).
This removed BKL but also made distributed filesystems very difficult
to implement leasing, which
typically needs to sleep for network communications (considering the cifs case).

Commit 1c8c601a changes this into inode->i_lock but it's still a
spinlock. And since i_lock is widely
used. The f_op->setlease() implementer needs to be even more careful
to avoid a recursive spinlock in
calling other VFS functions.

So would you consider to remove the spinlock around f_op->setlease()?
I believe here the spinlock is
used to protect the returned "struct file_lock **flp".  I think it
could be acquired in the enter of
generic_setlease() but not be released if (and only if) the
generic_setlease() returned successfully.

This can make the filesystems do anything they want before calling
generic_setlease(). Of course
the code after generic_setlease() still need to be careful, but it's
much easier. Especially, if there
are any errors happened, the filesystem need to release the lock
explicity and then call __break_lease().

Or we can simply change spinlock into mutex, or any mechanism which is
safe for sleeping?

Copy to Jeff and Arnd who were working on that lock. Also Miklos, yes,
it's still me, the one was
worked on fuse adaptive readdir_plus....now I'm working for GPFS....

-- Feng Shuo

Re: Considering remove spinlock around f_op->setlease()

[Christoph Hellwig]

On Fri, Aug 16, 2013 at 12:41:14AM +0800, Feng Shuo wrote:
> much easier. Especially, if there
> Copy to Jeff and Arnd who were working on that lock. Also Miklos, yes,
> it's still me, the one was
> worked on fuse adaptive readdir_plus....now I'm working for GPFS....

People will start giving a shit once your filesystem in the Linux source
tree.  People that violate our license are generally not overly liked.

Re: Considering remove spinlock around f_op->setlease()

[Jeff Layton]

On Fri, 16 Aug 2013 05:17:26 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Fri, Aug 16, 2013 at 12:41:14AM +0800, Feng Shuo wrote:
> > much easier. Especially, if there
> > Copy to Jeff and Arnd who were working on that lock. Also Miklos, yes,
> > it's still me, the one was
> > worked on fuse adaptive readdir_plus....now I'm working for GPFS....
> 
> People will start giving a shit once your filesystem in the Linux source
> tree.  People that violate our license are generally not overly liked.
> 

I concur. I'm not sure you'll get much traction on this since no
existing ->setlease caller in the tree needs it.

The only current ->setlease caller that actually does anything is
cifs_setlease, and it will only succeed if there's already an oplock on
the file. Eventually we might want to add support for upgrading SMB2/3
leases there, and at that point we probably will need to do something
that allows that function to sleep.

I know that Steve Whitehouse had also mentioned implementing leases in
GFS2 at some point, but no idea whether that will involve operations
that need to sleep (I imagine it will since you'll probably need to
talk to DLM).

So, I have no specific objection to sane patches that allow ->setlease
to be called without the lock held, but you do need a way to ensure
that the returned lease will stick around until you can do the fasync
stuff. I don't think we want to convert all of this to mutexes, so
you'll probably need to get more clever with how the spinlocking is
handled.

-- 
Jeff Layton <jlayton@redhat.com>

Re: Considering remove spinlock around f_op->setlease()

[Feng Shuo]

Well, I don't quite understand, but typically, I don't give shit to
anything other than toilet.

Before join GPFS, I had some experience with other distributed
filesystems. I believe NFSv4 (with callback support), CIFS and other
distributed filesystems have the ability to support leasing, but look
at current cifs code (cifs_setlease() in fs/cifs/cifsfs.c). To avoid
sleeping, it actually only works if everything is cached (with oplock
acquired). I don't think this really helps because the application
will be so easy to get -EAGAIN to do anything.

So I think, generally, to make distributed filesystem provide a real
working leasing mechanism, we might still need to think about changing
this lock.

On Fri, Aug 16, 2013 at 8:17 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Aug 16, 2013 at 12:41:14AM +0800, Feng Shuo wrote:
>> much easier. Especially, if there
>> Copy to Jeff and Arnd who were working on that lock. Also Miklos, yes,
>> it's still me, the one was
>> worked on fuse adaptive readdir_plus....now I'm working for GPFS....
>
> People will start giving a shit once your filesystem in the Linux source
> tree.  People that violate our license are generally not overly liked.
>