gfs2: fix lock cancelling
PATCH 1/1 gfs2: fix lock cancelling
We're cancelling blocked locks by sending an additional unlock request. This is not correct in general; for example, if the lock request was a downgrade from a write to a read (posix supports atomic upgrades and downgrades), then the result of an unlock will leave the file unlocked, whereas a cancelled downgrade should leave the file locked.
So, modify the gfs2 lock cancel operation to just remove the cancelled lock from any lists, or return an error if it's too late and the lock request has already completed (though not necessarily succeeded).
If gfs2's lock manager returns a result after we've cancelled the request, the lock manager will receive an error on the write that returns the result. The lock manager is then responsible for cancelling the request.
--- fs/gfs2/lm.c | 10 +++++++++ fs/gfs2/locking/dlm/mount.c | 1 + fs/gfs2/locking/dlm/plock.c | 44 +++++++++++++++++++++++++++++++++++++--- fs/gfs2/locking/nolock/main.c | 16 +++++++++----- fs/gfs2/ops_file.c | 7 +---- include/linux/lm_interface.h | 3 ++ 6 files changed, 66 insertions(+), 15 deletions(-)
Does this look reasonable? (Compile-tested only for now....)--b.
fs/gfs2/lm.c +10 -0
@@ -200,6 +200,16 @@ int gfs2_lm_punlock(struct gfs2_sbd *sdp, struct lm_lockname *name,return error; } +int gfs2_lm_pcancel (struct gfs2_sbd *sdp,struct lm_lockname *name, +struct file *file,struct file_lock *fl) +{ +int error = -EIO ; +if (likely (!test_bit (SDF_SHUTDOWN , &sdp->sd_flags))) + error = sdp->sd_lockstruct.ls_ops->lm_pcancel ( + sdp->sd_lockstruct.ls_lockspace, name, file, fl); +return error; +} +void gfs2_lm_recovery_done (struct gfs2_sbd *sdp,unsigned int jid,unsigned int message) {
fs/gfs2/locking/dlm/mount.c +1 -0
@@ -244,6 +244,7 @@ const struct lm_lockops gdlm_ops = { .lm_plock = gdlm_plock, .lm_punlock = gdlm_punlock, .lm_plock_get = gdlm_plock_get, + .lm_pcancel = gdlm_pcancel, .lm_cancel = gdlm_cancel, .lm_hold_lvb = gdlm_hold_lvb, .lm_unhold_lvb = gdlm_unhold_lvb,
fs/gfs2/locking/dlm/plock.c +40 -4
74 lines not on this page · raw message
fs/gfs2/locking/nolock/main.c +10 -6
@@ -165,23 +165,26 @@ static int nolock_plock_get(void *lockspace, struct lm_lockname *name, {posix_test_lock (file, fl); +/* XXX: Just make the prototype a void ? */ return 0 ; }static int nolock_plock (void *lockspace,struct lm_lockname *name,struct file *file,int cmd,struct file_lock *fl) { -int error; - error =posix_lock_file_wait (file, fl); -return error; +return posix_lock_file_wait (file, fl); }static int nolock_punlock (void *lockspace,struct lm_lockname *name,struct file *file,struct file_lock *fl) { -int error; - error =posix_lock_file_wait (file, fl); -return error; +return posix_lock_file_wait (file, fl); +} + +static int nolock_pcancel (void *lockspace,struct lm_lockname *name, +struct file *file,struct file_lock *fl) +{ +return 0 ; } static void nolock_recovery_done(void *lockspace, unsigned int jid,
@@ -205,6 +208,7 @@ static const struct lm_lockops nolock_ops = { .lm_plock_get = nolock_plock_get, .lm_plock = nolock_plock, .lm_punlock = nolock_punlock, + .lm_pcancel = nolock_pcancel, .lm_recovery_done = nolock_recovery_done, .lm_owner = THIS_MODULE, };
fs/gfs2/ops_file.c +2 -5
@@ -547,11 +547,8 @@ static int gfs2_lock(struct file *file, int cmd, struct file_lock *fl) } } -if (cmd ==F_CANCELLK ) { -/* Hack: */ - cmd =F_SETLK ; - fl->fl_type =F_UNLCK ; - } +if (cmd ==F_CANCELLK ) +return gfs2_lm_pcancel (sdp, &name, file, fl);if (IS_GETLK (cmd))return gfs2_lm_plock_get (sdp, &name, file, fl);else if (fl->fl_type ==F_UNLCK )
include/linux/lm_interface.h +3 -0
@@ -216,6 +216,9 @@ struct lm_lockops {int (*lm_punlock) (void *lockspace,struct lm_lockname *name,struct file *file,struct file_lock *fl); +int (*lm_pcancel) (void *lockspace,struct lm_lockname *name, +struct file *file,struct file_lock *fl); +/* * Client oriented operations */
Signature · 1 line
1.5.3.1.139.g9346b
PATCH 1/1 gfs2: fix lock cancelling
Quoted 7 lines · J. Bruce Fields
fs/gfs2/locking/dlm/plock.c +7
-0+int gdlm_plock_cancel (void *lockspace,struct lm_lockname *name, +struct file *file,struct file_lock *fl) +{ +struct gdlm_ls *ls = lockspace; +struct plock_xop *xop; +struct plock_op *op; +
fs/gfs2/locking/dlm/plock.c int gdlm_plock_get(void *lockspace, struct lm_lockname *name, · line 293
Quoted 10 lines
fs/gfs2/locking/dlm/plock.c +10
-0+spin_lock (&ops_lock); +list_for_each_entry (op, &recv_list, list) { + xop = (struct plock_xop *xop)op; +if (!xop->callback) +continue ; +if (xop->fl != fl) +continue ; +list_del_init (&op->list); + goto found; + }
If found on the recv_list, it means the op has been sent up to the lock manager in userspace and is still floating around up there. If we remove the op from the recv_list, it means, as you say, that the lock manager could get an error back later when it does dev_write() to complete the op. (dev_write() just prints an error message currently, doesn't return an error to userspace.)
This assumes, of course, that seeing an error, the lock manager could do something sensible to bring itself back in sync with the application... as we've discussed before, that's a hard problem that we may never solve :-)
fs/gfs2/locking/dlm/plock.c int gdlm_plock_get(void *lockspace, struct lm_lockname *name, · line 302
Quoted 9 lines
fs/gfs2/locking/dlm/plock.c +9
-0+list_for_each_entry (op, &send_list, list) { + xop = (struct plock_xop *xop)op; +if (!xop->callback) +continue ; +if (xop->fl != fl) +continue ; +list_del_init (&op->list); + goto found; + }
If found on the send_list, it means the op hasn't been sent up to the lock manager yet, so the cancel can be considered a success.
fs/gfs2/locking/dlm/plock.c int gdlm_plock_get(void *lockspace, struct lm_lockname *name, · line 305
Quoted 3 lines
fs/gfs2/locking/dlm/plock.c +3
-0+spin_unlock (&ops_lock); +/* Too late; the lock's probably already been granted. */ +return -ENOENT ;
It's up to the caller to sort out what happens in this case.
Quoted 7 lines
fs/gfs2/locking/dlm/plock.c +7
-0+found: +spin_unlock (&ops_lock); +/* XXX: Is any other cleanup necessary here? */ +kfree (op); +return 0 ; +} +
PATCH 1/1 gfs2: fix lock cancelling
Quoted 10 lines · David Teigland
If found on the
recv_list, it means the op has been sent up to the lock manager in userspace and is still floating around up there. If we remove the op from therecv_list, it means, as you say, that the lock manager could get an error back later when it doesdev_write()to complete the op. (dev_write()just prints an error message currently, doesn't return an error to userspace.)This assumes, of course, that seeing an error, the lock manager could do something sensible to bring itself back in sync with the application... as we've discussed before, that's a hard problem that we may never solve :-)
It's a hard problem, but it'll need to be solved some day. And it can't be solved as long as the kernel isn't even giving userspace the information it would need to solve the problem.
For now, could you just generate an unlock request in the case where you get an error on the write? That's certainly not perfect, but it's no worse than the current behavior.
Quoted 13 lines
fs/gfs2/locking/dlm/plock.c int gdlm_plock_get(void *lockspace, struct lm_lockname *name, · line 302
Quoted 9 lines
fs/gfs2/locking/dlm/plock.c +9
-0+list_for_each_entry (op, &send_list, list) { + xop = (struct plock_xop *xop)op; +if (!xop->callback) +continue ; +if (xop->fl != fl) +continue ; +list_del_init (&op->list); + goto found; + }If found on the
send_list, it means the op hasn't been sent up to the lock manager yet, so the cancel can be considered a success.
Quoted 5 lines
fs/gfs2/locking/dlm/plock.c int gdlm_plock_get(void *lockspace, struct lm_lockname *name, · line 305
Quoted 3 lines
fs/gfs2/locking/dlm/plock.c +3
-0+spin_unlock (&ops_lock); +/* Too late; the lock's probably already been granted. */ +return -ENOENT ;It's up to the caller to sort out what happens in this case.
Yup.
--b.
PATCH 1/1 gfs2: fix lock cancelling
Quoted 12 lines · J. Bruce Fields
Quoted 10 lines · David Teigland
If found on the
recv_list, it means the op has been sent up to the lock manager in userspace and is still floating around up there. If we remove the op from therecv_list, it means, as you say, that the lock manager could get an error back later when it doesdev_write()to complete the op. (dev_write()just prints an error message currently, doesn't return an error to userspace.)This assumes, of course, that seeing an error, the lock manager could do something sensible to bring itself back in sync with the application... as we've discussed before, that's a hard problem that we may never solve :-)
Quoted 7 lines
It's a hard problem, but it'll need to be solved some day. And it can't be solved as long as the kernel isn't even giving userspace the information it would need to solve the problem.
For now, could you just generate an unlock request in the case where you get an error on the write? That's certainly not perfect, but it's no worse than the current behavior.
Oh certainly, I have no problem with making our best attempt. Under certain conditions it may work well enough to be fine. And in the future we may find ways for the lock manager to do better.
Dave