I could be missing something blatantly obvious so please be kind...
I would like to understand how one _should_ implement ->lock() for a
filesystem that can't immediately determine that the request would
result in -EDEADLK or -ENOLCK. Particularly for deferred SETLKW
requests.
That is, the filesystem's ->lock() initially responds with
-EINPROGRESS but later goes on to fail the request with -EDEADLK or
-ENOLCK. The filesystem's call to ->fl_grant() will be made with an
unsupported 'result'.
Having reviewed the code in fs/lockd/svclock.c it would seem that it
was never designed to allow EDEADLK or ENOLCK _after_ nlmsvc_lock()'s
initial call to vfs_lock_file().
nlmsvc_grant_deferred() properly handles deferred SETLKW processing
IFF the result is 0 (aka granted).
nlmsvc_grant_deferred()'s SETLK processing will at least return
failures (but they are assumed to be B_TIMED_OUT).
In the end the real 'result' gets dropped on the floor.
Why must ->lock() immediately return error (e.g. EDEADLK), otherwise
it is assumed the request will complete successfully (or timeout in
the B_QUEUED/SETLK case)? Seems overly constrained (painfully so for
SETLKW) when you consider that the whole point of deferred processing
(via EINPROGRESS and ->fl_grant) is to accomodate filesystems that
can't respond to a lock request immediately.
regards,
Mike
On Tue, Dec 18, 2007 at 11:03:01PM -0500, Mike Snitzer wrote:
> I could be missing something blatantly obvious so please be kind...
>
> I would like to understand how one _should_ implement ->lock() for a
> filesystem that can't immediately determine that the request would
> result in -EDEADLK or -ENOLCK. Particularly for deferred SETLKW
> requests.
>
> That is, the filesystem's ->lock() initially responds with
> -EINPROGRESS but later goes on to fail the request with -EDEADLK or
> -ENOLCK. The filesystem's call to ->fl_grant() will be made with an
> unsupported 'result'.
>
> Having reviewed the code in fs/lockd/svclock.c it would seem that it
> was never designed to allow EDEADLK or ENOLCK _after_ nlmsvc_lock()'s
> initial call to vfs_lock_file().
>
> nlmsvc_grant_deferred() properly handles deferred SETLKW processing
> IFF the result is 0 (aka granted).
> nlmsvc_grant_deferred()'s SETLK processing will at least return
> failures (but they are assumed to be B_TIMED_OUT).
> In the end the real 'result' gets dropped on the floor.
>
> Why must ->lock() immediately return error (e.g. EDEADLK), otherwise
> it is assumed the request will complete successfully (or timeout in
> the B_QUEUED/SETLK case)? Seems overly constrained (painfully so for
> SETLKW) when you consider that the whole point of deferred processing
> (via EINPROGRESS and ->fl_grant) is to accomodate filesystems that
> can't respond to a lock request immediately.
Yeah, the assuumption was that we only needed to deal with non-blocking
requests first, under the theory that the blocking case could always be
handled with a deny followed by an eventual grant. Which, as you point
out, may not ever be coming in the EDEALK or ENOLCK cases. I'm inclined
to agree it's ugly.
That said, I find it hard to care about the EDEADLK case--deadlock
detection just isn't going to be reliable in the presence of nfs clients
anyway. And are there really a lot of cases where ENOLCK has to be
returned and where the local filesystem doesn't know that immediately?
But it should probably be rethought anyway. I'm grateful for any
suggestions (but probably won't be working on this seriously until the
new year).
--b.