On Sat, 17 Jan 2015 07:45:13 -0600
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Thu, Jan 15, 2015 at 6:22 AM, Sachin Prabhu <sprabhu@xxxxxxxxxx> wrote:
> > Commit
> > c11f1df5003d534fd067f0168bfad7befffb3b5c
> > requires writers to wait for any pending oplock break handler to
> > complete before proceeding to write. This is done by waiting on bit
> > CIFS_INODE_PENDING_OPLOCK_BREAK in cifsFileInfo->flags. This bit is
> > cleared by the oplock break handler job queued on the workqueue once it
> > has completed handling the oplock break allowing writers to proceed with
> > writing to the file.
> >
> > While testing, it was noticed that the filehandle could be closed while
> > there is a pending oplock break which results in the oplock break
> > handler on the cifsiod workqueue being cancelled before it has had a
> > chance to execute and clear the CIFS_INODE_PENDING_OPLOCK_BREAK bit.
> > Any subsequent attempt to write to this file hangs waiting for the
> > CIFS_INODE_PENDING_OPLOCK_BREAK bit to be cleared.
> >
> > We fix this by ensuring that we also clear the bit
> > CIFS_INODE_PENDING_OPLOCK_BREAK when we remove the oplock break handler
> > from the workqueue.
> >
> > The bug was found by Red Hat QA while testing using ltp's fsstress
> > command.
> >
> > Signed-off-by: Sachin Prabhu <sprabhu@xxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxx>
> > ---
> > fs/cifs/file.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 96b7e9b..74f1287 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -366,6 +366,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> > struct cifsLockInfo *li, *tmp;
> > struct cifs_fid fid;
> > struct cifs_pending_open open;
> > + bool oplock_break_cancelled;
> >
> > spin_lock(&cifs_file_list_lock);
> > if (--cifs_file->count > 0) {
> > @@ -397,7 +398,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> > }
> > spin_unlock(&cifs_file_list_lock);
> >
> > - cancel_work_sync(&cifs_file->oplock_break);
> > + oplock_break_cancelled = cancel_work_sync(&cifs_file->oplock_break);
> >
> > if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
> > struct TCP_Server_Info *server = tcon->ses->server;
> > @@ -409,6 +410,9 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> > _free_xid(xid);
> > }
> >
> > + if (oplock_break_cancelled)
> > + cifs_done_oplock_break(cifsi);
> > +
> > cifs_del_pending_open(&open);
> >
> > /*
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> Does it matter what cancel_work_sync() returns?
> Should cifs_done_oplock_break(cifsi); be called unconditionally?
I guess the question is whether you can end up with another oplock
break racing in between cancel_work_sync and the
cifs_done_oplock_break. If that can't happen, then calling it
unconditionally is fine.
> Also, should this also to go stable?
Yes, probably. It can cause the client to hang.
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
