| Message ID | 20130308153002.GC1511@mguzik.brq.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 8 Mar 2013 16:30:03 +0100 Mateusz Guzik <mguzik@redhat.com> wrote: > cifsFileInfo objects hold references to dentries and it is possible that > these will still be around in workqueues when VFS decides to kill super > block during unmount. > FWIW, this doesn't even need to be sitting in a workqueue when it comes to readahead. We may have sprayed out a bunch of readahead requests and are simply waiting on the replies. We hold a reference to the dentry at that point, but not one to the vfsmount, so a umount can get started before those replies come in. That's almost certainly the problem that Ben hit... > This results in panics like this one: > BUG: Dentry ffff88001f5e76c0{i=66b4a,n=1M-2} still in use (1) [unmount of cifs cifs] > ------------[ cut here ]------------ > kernel BUG at fs/dcache.c:943! > [..] > Process umount (pid: 1781, threadinfo ffff88003d6e8000, task ffff880035eeaec0) > [..] > Call Trace: > [<ffffffff811b44f3>] shrink_dcache_for_umount+0x33/0x60 > [<ffffffff8119f7fc>] generic_shutdown_super+0x2c/0xe0 > [<ffffffff8119f946>] kill_anon_super+0x16/0x30 > [<ffffffffa036623a>] cifs_kill_sb+0x1a/0x30 [cifs] > [<ffffffff8119fcc7>] deactivate_locked_super+0x57/0x80 > [<ffffffff811a085e>] deactivate_super+0x4e/0x70 > [<ffffffff811bb417>] mntput_no_expire+0xd7/0x130 > [<ffffffff811bc30c>] sys_umount+0x9c/0x3c0 > [<ffffffff81657c19>] system_call_fastpath+0x16/0x1b > > Fix this by making each cifsFileInfo object hold a reference to cifs > super block, which implicitly keeps VFS super block around as well. > > Signed-off-by: Mateusz Guzik <mguzik@redhat.com> > Reviewed-by: Jeff Layton <jlayton@redhat.com> > Reported-and-Tested-by: Ben Greear <greearb@candelatech.com> > --- > fs/cifs/cifsfs.c | 24 ++++++++++++++++++++++++ > fs/cifs/cifsfs.h | 4 ++++ > fs/cifs/file.c | 6 +++++- > 3 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index 1a052c0..054b90b 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -91,6 +91,30 @@ struct workqueue_struct *cifsiod_wq; > __u8 cifs_client_guid[SMB2_CLIENT_GUID_SIZE]; > #endif > > +/* > + * Bumps refcount for cifs super block. > + * Note that it should be only called if a referece to VFS super block is > + * already held, e.g. in open-type syscalls context. Otherwise it can race with > + * atomic_dec_and_test in deactivate_locked_super. > + */ > +void > +cifs_sb_active(struct super_block *sb) > +{ > + struct cifs_sb_info *server = CIFS_SB(sb); > + > + if (atomic_inc_return(&server->active) == 1) > + atomic_inc(&sb->s_active); > +} > + > +void > +cifs_sb_deactive(struct super_block *sb) > +{ > + struct cifs_sb_info *server = CIFS_SB(sb); > + > + if (atomic_dec_and_test(&server->active)) > + deactivate_super(sb); > +} > + > static int > cifs_read_super(struct super_block *sb) > { > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h > index 7163419..0e32c34 100644 > --- a/fs/cifs/cifsfs.h > +++ b/fs/cifs/cifsfs.h > @@ -41,6 +41,10 @@ extern struct file_system_type cifs_fs_type; > extern const struct address_space_operations cifs_addr_ops; > extern const struct address_space_operations cifs_addr_ops_smallbuf; > > +/* Functions related to super block operations */ > +extern void cifs_sb_active(struct super_block *sb); > +extern void cifs_sb_deactive(struct super_block *sb); > + > /* Functions related to inodes */ > extern const struct inode_operations cifs_dir_inode_ops; > extern struct inode *cifs_root_iget(struct super_block *); > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 8c0d855..7a0dd99 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -300,6 +300,8 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, > INIT_WORK(&cfile->oplock_break, cifs_oplock_break); > mutex_init(&cfile->fh_mutex); > > + cifs_sb_active(inode->i_sb); > + > /* > * If the server returned a read oplock and we have mandatory brlocks, > * set oplock level to None. > @@ -349,7 +351,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) > struct cifs_tcon *tcon = tlink_tcon(cifs_file->tlink); > struct TCP_Server_Info *server = tcon->ses->server; > struct cifsInodeInfo *cifsi = CIFS_I(inode); > - struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb); > + struct super_block *sb = inode->i_sb; > + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > struct cifsLockInfo *li, *tmp; > struct cifs_fid fid; > struct cifs_pending_open open; > @@ -414,6 +417,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) > > cifs_put_tlink(cifs_file->tlink); > dput(cifs_file->dentry); > + cifs_sb_deactive(sb); > kfree(cifs_file); > } > Nice work. Steve, I think this ought to go into 3.9, and is probably a stable candidate as well.
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 1a052c0..054b90b 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -91,6 +91,30 @@ struct workqueue_struct *cifsiod_wq; __u8 cifs_client_guid[SMB2_CLIENT_GUID_SIZE]; #endif +/* + * Bumps refcount for cifs super block. + * Note that it should be only called if a referece to VFS super block is + * already held, e.g. in open-type syscalls context. Otherwise it can race with + * atomic_dec_and_test in deactivate_locked_super. + */ +void +cifs_sb_active(struct super_block *sb) +{ + struct cifs_sb_info *server = CIFS_SB(sb); + + if (atomic_inc_return(&server->active) == 1) + atomic_inc(&sb->s_active); +} + +void +cifs_sb_deactive(struct super_block *sb) +{ + struct cifs_sb_info *server = CIFS_SB(sb); + + if (atomic_dec_and_test(&server->active)) + deactivate_super(sb); +} + static int cifs_read_super(struct super_block *sb) { diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h index 7163419..0e32c34 100644 --- a/fs/cifs/cifsfs.h +++ b/fs/cifs/cifsfs.h @@ -41,6 +41,10 @@ extern struct file_system_type cifs_fs_type; extern const struct address_space_operations cifs_addr_ops; extern const struct address_space_operations cifs_addr_ops_smallbuf; +/* Functions related to super block operations */ +extern void cifs_sb_active(struct super_block *sb); +extern void cifs_sb_deactive(struct super_block *sb); + /* Functions related to inodes */ extern const struct inode_operations cifs_dir_inode_ops; extern struct inode *cifs_root_iget(struct super_block *); diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 8c0d855..7a0dd99 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -300,6 +300,8 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, INIT_WORK(&cfile->oplock_break, cifs_oplock_break); mutex_init(&cfile->fh_mutex); + cifs_sb_active(inode->i_sb); + /* * If the server returned a read oplock and we have mandatory brlocks, * set oplock level to None. @@ -349,7 +351,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) struct cifs_tcon *tcon = tlink_tcon(cifs_file->tlink); struct TCP_Server_Info *server = tcon->ses->server; struct cifsInodeInfo *cifsi = CIFS_I(inode); - struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb); + struct super_block *sb = inode->i_sb; + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); struct cifsLockInfo *li, *tmp; struct cifs_fid fid; struct cifs_pending_open open; @@ -414,6 +417,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) cifs_put_tlink(cifs_file->tlink); dput(cifs_file->dentry); + cifs_sb_deactive(sb); kfree(cifs_file); }
cifsFileInfo objects hold references to dentries and it is possible that these will still be around in workqueues when VFS decides to kill super block during unmount. This results in panics like this one: BUG: Dentry ffff88001f5e76c0{i=66b4a,n=1M-2} still in use (1) [unmount of cifs cifs] ------------[ cut here ]------------ kernel BUG at fs/dcache.c:943! [..] Process umount (pid: 1781, threadinfo ffff88003d6e8000, task ffff880035eeaec0) [..] Call Trace: [<ffffffff811b44f3>] shrink_dcache_for_umount+0x33/0x60 [<ffffffff8119f7fc>] generic_shutdown_super+0x2c/0xe0 [<ffffffff8119f946>] kill_anon_super+0x16/0x30 [<ffffffffa036623a>] cifs_kill_sb+0x1a/0x30 [cifs] [<ffffffff8119fcc7>] deactivate_locked_super+0x57/0x80 [<ffffffff811a085e>] deactivate_super+0x4e/0x70 [<ffffffff811bb417>] mntput_no_expire+0xd7/0x130 [<ffffffff811bc30c>] sys_umount+0x9c/0x3c0 [<ffffffff81657c19>] system_call_fastpath+0x16/0x1b Fix this by making each cifsFileInfo object hold a reference to cifs super block, which implicitly keeps VFS super block around as well. Signed-off-by: Mateusz Guzik <mguzik@redhat.com> Reviewed-by: Jeff Layton <jlayton@redhat.com> Reported-and-Tested-by: Ben Greear <greearb@candelatech.com> --- fs/cifs/cifsfs.c | 24 ++++++++++++++++++++++++ fs/cifs/cifsfs.h | 4 ++++ fs/cifs/file.c | 6 +++++- 3 files changed, 33 insertions(+), 1 deletion(-)