| Message ID | 20200624071053.993784-1-yangerkun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: remove the retry in cifs_poxis_lock_set | expand |
Ping... 在 2020/6/24 15:10, yangerkun 写道: > The caller of cifs_posix_lock_set will do retry(like > fcntl_setlk64->do_lock_file_wait) if we will wait for any file_lock. > So the retry in cifs_poxis_lock_set seems duplicated, remove it to > make a cleanup. > > Signed-off-by: yangerkun <yangerkun@huawei.com> > --- > fs/cifs/file.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 9b0f8f33f832..2c9c24b1805d 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -1162,7 +1162,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) > if ((flock->fl_flags & FL_POSIX) == 0) > return rc; > > -try_again: > cifs_down_write(&cinode->lock_sem); > if (!cinode->can_cache_brlcks) { > up_write(&cinode->lock_sem); > @@ -1171,13 +1170,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) > > rc = posix_lock_file(file, flock, NULL); > up_write(&cinode->lock_sem); > - if (rc == FILE_LOCK_DEFERRED) { > - rc = wait_event_interruptible(flock->fl_wait, > - list_empty(&flock->fl_blocked_member)); > - if (!rc) > - goto try_again; > - locks_delete_block(flock); > - } > return rc; > } > >
On Tue, Jun 30 2020, yangerkun wrote: > Ping... > > 在 2020/6/24 15:10, yangerkun 写道: >> The caller of cifs_posix_lock_set will do retry(like >> fcntl_setlk64->do_lock_file_wait) if we will wait for any file_lock. >> So the retry in cifs_poxis_lock_set seems duplicated, remove it to >> make a cleanup. If cifs_posix_try_lock() returns FILE_LOCK_DEFERRED (which it might after your patch), then cifs_setlk() will check the return value: if (!rc || rc < 0) return rc; These tests will fail (as FILE_LOCK_DEFERRED is 1) and so it will continue on as though the lock was granted. So I think your patch is wrong. However I think your goal is correct. cifs shouldn't be waiting. No other filesystem waits when it gets FILE_LOCK_DEFERRED. So maybe try to fix up your patch. Thanks, NeilBrown >> >> Signed-off-by: yangerkun <yangerkun@huawei.com> >> --- >> fs/cifs/file.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c >> index 9b0f8f33f832..2c9c24b1805d 100644 >> --- a/fs/cifs/file.c >> +++ b/fs/cifs/file.c >> @@ -1162,7 +1162,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) >> if ((flock->fl_flags & FL_POSIX) == 0) >> return rc; >> >> -try_again: >> cifs_down_write(&cinode->lock_sem); >> if (!cinode->can_cache_brlcks) { >> up_write(&cinode->lock_sem); >> @@ -1171,13 +1170,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) >> >> rc = posix_lock_file(file, flock, NULL); >> up_write(&cinode->lock_sem); >> - if (rc == FILE_LOCK_DEFERRED) { >> - rc = wait_event_interruptible(flock->fl_wait, >> - list_empty(&flock->fl_blocked_member)); >> - if (!rc) >> - goto try_again; >> - locks_delete_block(flock); >> - } >> return rc; >> } >> >>
在 2020/7/1 6:34, NeilBrown 写道: > On Tue, Jun 30 2020, yangerkun wrote: > >> Ping... >> >> 在 2020/6/24 15:10, yangerkun 写道: >>> The caller of cifs_posix_lock_set will do retry(like >>> fcntl_setlk64->do_lock_file_wait) if we will wait for any file_lock. >>> So the retry in cifs_poxis_lock_set seems duplicated, remove it to >>> make a cleanup. > > If cifs_posix_try_lock() returns FILE_LOCK_DEFERRED (which it might > after your patch), then cifs_setlk() will check the return value: > > if (!rc || rc < 0) > return rc; > > These tests will fail (as FILE_LOCK_DEFERRED is 1) and so it will > continue on as though the lock was granted. > > So I think your patch is wrong. > However I think your goal is correct. cifs shouldn't be waiting. > No other filesystem waits when it gets FILE_LOCK_DEFERRED. > > So maybe try to fix up your patch. Yes, we should check FILE_LOCK_DEFERRED in cifs_setlk after this patch. Also we may change 'int rc = 1;' exists in cifs_posix_lock_set since FILE_LOCK_DEFERRED equals to 1. I will send a v2 and thanks a lot for your review! Thanks, Kun. > Thanks, > NeilBrown > > >>> >>> Signed-off-by: yangerkun <yangerkun@huawei.com> >>> --- >>> fs/cifs/file.c | 8 -------- >>> 1 file changed, 8 deletions(-) >>> >>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c >>> index 9b0f8f33f832..2c9c24b1805d 100644 >>> --- a/fs/cifs/file.c >>> +++ b/fs/cifs/file.c >>> @@ -1162,7 +1162,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) >>> if ((flock->fl_flags & FL_POSIX) == 0) >>> return rc; >>> >>> -try_again: >>> cifs_down_write(&cinode->lock_sem); >>> if (!cinode->can_cache_brlcks) { >>> up_write(&cinode->lock_sem); >>> @@ -1171,13 +1170,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) >>> >>> rc = posix_lock_file(file, flock, NULL); >>> up_write(&cinode->lock_sem); >>> - if (rc == FILE_LOCK_DEFERRED) { >>> - rc = wait_event_interruptible(flock->fl_wait, >>> - list_empty(&flock->fl_blocked_member)); >>> - if (!rc) >>> - goto try_again; >>> - locks_delete_block(flock); >>> - } >>> return rc; >>> } >>> >>>
diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 9b0f8f33f832..2c9c24b1805d 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -1162,7 +1162,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) if ((flock->fl_flags & FL_POSIX) == 0) return rc; -try_again: cifs_down_write(&cinode->lock_sem); if (!cinode->can_cache_brlcks) { up_write(&cinode->lock_sem); @@ -1171,13 +1170,6 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) rc = posix_lock_file(file, flock, NULL); up_write(&cinode->lock_sem); - if (rc == FILE_LOCK_DEFERRED) { - rc = wait_event_interruptible(flock->fl_wait, - list_empty(&flock->fl_blocked_member)); - if (!rc) - goto try_again; - locks_delete_block(flock); - } return rc; }
The caller of cifs_posix_lock_set will do retry(like fcntl_setlk64->do_lock_file_wait) if we will wait for any file_lock. So the retry in cifs_poxis_lock_set seems duplicated, remove it to make a cleanup. Signed-off-by: yangerkun <yangerkun@huawei.com> --- fs/cifs/file.c | 8 -------- 1 file changed, 8 deletions(-)