| Message ID | 20241120160154.1502662-1-paul@darkrain42.org |
|---|---|
| State | New |
| Headers | show |
| Series | smb: Log an error when close_all_cached_dirs fails | expand |
Was looking through old patches and noticed this dir_lease patch wasn't merged. Any thoughts on whether it is still valid and worth merging? Could be important given the recent work on dir leases On Wed, Nov 20, 2024 at 10:02 AM Paul Aurich <paul@darkrain42.org> wrote: > > Under low-memory conditions, close_all_cached_dirs() can't move the > dentries to a separate list to dput() them once the locks are dropped. > This will result in a "Dentry still in use" error, so add an error > message that makes it clear this is what happened: > > [ 495.281119] CIFS: VFS: \\otters.example.com\share Out of memory while dropping dentries > [ 495.281595] ------------[ cut here ]------------ > [ 495.281887] BUG: Dentry ffff888115531138{i=78,n=/} still in use (2) [unmount of cifs cifs] > [ 495.282391] WARNING: CPU: 1 PID: 2329 at fs/dcache.c:1536 umount_check+0xc8/0xf0 > > Also, bail out of looping through all tcons as soon as a single > allocation fails, since we're already in trouble, and kmalloc() attempts > for subseqeuent tcons are likely to fail just like the first one did. > > Signed-off-by: Paul Aurich <paul@darkrain42.org> > Suggested-by: Ruben Devos <rdevos@oxya.com> > Cc: stable@vger.kernel.org > --- > > This could also be folded directly into "smb: During unmount, ensure all > cached dir instances drop their dentry". > > --- > fs/smb/client/cached_dir.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c > index 004349a7ab69..8b510c858f4f 100644 > --- a/fs/smb/client/cached_dir.c > +++ b/fs/smb/client/cached_dir.c > @@ -488,12 +488,21 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb) > if (cfids == NULL) > continue; > spin_lock(&cfids->cfid_list_lock); > list_for_each_entry(cfid, &cfids->entries, entry) { > tmp_list = kmalloc(sizeof(*tmp_list), GFP_ATOMIC); > - if (tmp_list == NULL) > - break; > + if (tmp_list == NULL) { > + /* > + * If the malloc() fails, we won't drop all > + * dentries, and unmounting is likely to trigger > + * a 'Dentry still in use' error. > + */ > + cifs_tcon_dbg(VFS, "Out of memory while dropping dentries\n"); > + spin_unlock(&cfids->cfid_list_lock); > + spin_unlock(&cifs_sb->tlink_tree_lock); > + goto done; > + } > spin_lock(&cfid->fid_lock); > tmp_list->dentry = cfid->dentry; > cfid->dentry = NULL; > spin_unlock(&cfid->fid_lock); > > @@ -501,10 +510,11 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb) > } > spin_unlock(&cfids->cfid_list_lock); > } > spin_unlock(&cifs_sb->tlink_tree_lock); > > +done: > list_for_each_entry_safe(tmp_list, q, &entry, entry) { > list_del(&tmp_list->entry); > dput(tmp_list->dentry); > kfree(tmp_list); > } > -- > 2.45.2 > >
> -----Oorspronkelijk bericht----- > Van: Steve French <smfrench@gmail.com> > Verzonden: donderdag 19 juni 2025 5:37 > Aan: Paul Aurich <paul@darkrain42.org> > CC: linux-cifs@vger.kernel.org; Ruben Devos <rdevos@oxya.com>; Paulo > Alcantara <pc@manguebit.com>; Ronnie Sahlberg <ronniesahlberg@gmail.com>; > Shyam Prasad N <sprasad@microsoft.com>; Tom Talpey <tom@talpey.com>; > Bharath SM <bharathsm@microsoft.com>; Henrique Carvalho > <henrique.carvalho@suse.com> > Onderwerp: Re: [PATCH] smb: Log an error when close_all_cached_dirs fails > > > Was looking through old patches and noticed this dir_lease patch wasn't merged. > Any thoughts on whether it is still valid and worth merging? Hi Steve, I contacted Paul directly last year when I saw his patches for the dentry still in use issue. The extra message that's logged with this patch could be useful for sysadmins, it gives a bit more context to what happened. I will leave it up to the developers though to decide if still needs to be merged. > > Could be important given the recent work on dir leases > > > On Wed, Nov 20, 2024 at 10:02 AM Paul Aurich <paul@darkrain42.org> wrote: > > > > Under low-memory conditions, close_all_cached_dirs() can't move the > > dentries to a separate list to dput() them once the locks are dropped. > > This will result in a "Dentry still in use" error, so add an error > > message that makes it clear this is what happened: > > > > [ 495.281119] CIFS: VFS: \\otters.example.com\share Out of memory > > while dropping dentries [ 495.281595] ------------[ cut here > > ]------------ [ 495.281887] BUG: Dentry ffff888115531138{i=78,n=/} > > still in use (2) [unmount of cifs cifs] [ 495.282391] WARNING: CPU: 1 > > PID: 2329 at fs/dcache.c:1536 umount_check+0xc8/0xf0 > > > > Also, bail out of looping through all tcons as soon as a single > > allocation fails, since we're already in trouble, and kmalloc() > > attempts for subseqeuent tcons are likely to fail just like the first one did. > > > > Signed-off-by: Paul Aurich <paul@darkrain42.org> > > Suggested-by: Ruben Devos <rdevos@oxya.com> > > Cc: stable@vger.kernel.org > > --- > > > > This could also be folded directly into "smb: During unmount, ensure > > all cached dir instances drop their dentry". > > > > --- > > fs/smb/client/cached_dir.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c > > index 004349a7ab69..8b510c858f4f 100644 > > --- a/fs/smb/client/cached_dir.c > > +++ b/fs/smb/client/cached_dir.c > > @@ -488,12 +488,21 @@ void close_all_cached_dirs(struct cifs_sb_info > *cifs_sb) > > if (cfids == NULL) > > continue; > > spin_lock(&cfids->cfid_list_lock); > > list_for_each_entry(cfid, &cfids->entries, entry) { > > tmp_list = kmalloc(sizeof(*tmp_list), GFP_ATOMIC); > > - if (tmp_list == NULL) > > - break; > > + if (tmp_list == NULL) { > > + /* > > + * If the malloc() fails, we won't drop all > > + * dentries, and unmounting is likely to trigger > > + * a 'Dentry still in use' error. > > + */ > > + cifs_tcon_dbg(VFS, "Out of memory while dropping dentries\n"); > > + spin_unlock(&cfids->cfid_list_lock); > > + spin_unlock(&cifs_sb->tlink_tree_lock); > > + goto done; > > + } > > spin_lock(&cfid->fid_lock); > > tmp_list->dentry = cfid->dentry; > > cfid->dentry = NULL; > > spin_unlock(&cfid->fid_lock); > > > > @@ -501,10 +510,11 @@ void close_all_cached_dirs(struct cifs_sb_info > *cifs_sb) > > } > > spin_unlock(&cfids->cfid_list_lock); > > } > > spin_unlock(&cifs_sb->tlink_tree_lock); > > > > +done: > > list_for_each_entry_safe(tmp_list, q, &entry, entry) { > > list_del(&tmp_list->entry); > > dput(tmp_list->dentry); > > kfree(tmp_list); > > } > > -- > > 2.45.2 > > > > > > > -- > Thanks, > > Steve Ruben
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c index 004349a7ab69..8b510c858f4f 100644 --- a/fs/smb/client/cached_dir.c +++ b/fs/smb/client/cached_dir.c @@ -488,12 +488,21 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb) if (cfids == NULL) continue; spin_lock(&cfids->cfid_list_lock); list_for_each_entry(cfid, &cfids->entries, entry) { tmp_list = kmalloc(sizeof(*tmp_list), GFP_ATOMIC); - if (tmp_list == NULL) - break; + if (tmp_list == NULL) { + /* + * If the malloc() fails, we won't drop all + * dentries, and unmounting is likely to trigger + * a 'Dentry still in use' error. + */ + cifs_tcon_dbg(VFS, "Out of memory while dropping dentries\n"); + spin_unlock(&cfids->cfid_list_lock); + spin_unlock(&cifs_sb->tlink_tree_lock); + goto done; + } spin_lock(&cfid->fid_lock); tmp_list->dentry = cfid->dentry; cfid->dentry = NULL; spin_unlock(&cfid->fid_lock); @@ -501,10 +510,11 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb) } spin_unlock(&cfids->cfid_list_lock); } spin_unlock(&cifs_sb->tlink_tree_lock); +done: list_for_each_entry_safe(tmp_list, q, &entry, entry) { list_del(&tmp_list->entry); dput(tmp_list->dentry); kfree(tmp_list); }
Under low-memory conditions, close_all_cached_dirs() can't move the dentries to a separate list to dput() them once the locks are dropped. This will result in a "Dentry still in use" error, so add an error message that makes it clear this is what happened: [ 495.281119] CIFS: VFS: \\otters.example.com\share Out of memory while dropping dentries [ 495.281595] ------------[ cut here ]------------ [ 495.281887] BUG: Dentry ffff888115531138{i=78,n=/} still in use (2) [unmount of cifs cifs] [ 495.282391] WARNING: CPU: 1 PID: 2329 at fs/dcache.c:1536 umount_check+0xc8/0xf0 Also, bail out of looping through all tcons as soon as a single allocation fails, since we're already in trouble, and kmalloc() attempts for subseqeuent tcons are likely to fail just like the first one did. Signed-off-by: Paul Aurich <paul@darkrain42.org> Suggested-by: Ruben Devos <rdevos@oxya.com> Cc: stable@vger.kernel.org --- This could also be folded directly into "smb: During unmount, ensure all cached dir instances drop their dentry". --- fs/smb/client/cached_dir.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)