| Message ID | 20231108051027.12363-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: dcache: fix dget()/dget_dlock() kernel-doc | expand |
On Tue, Nov 07, 2023 at 09:10:27PM -0800, Randy Dunlap wrote: > + * The reference count increment in this function is not atomic. > + * Consider dget() if atomicity is required. No. dget() under ->d_lock will deadlock; dget_dlock() *not* under ->d_lock is a bug. There is nothing optional about that, so "consider" is seriously misleading. dget() is an equivalent of spin_lock(&dentry->d_lock); dentry->d_lockref.count++; spin_unlock(&dentry->d_lock); with a bit of an optimization that avoids 3 stores if it can get away with just one. Optimization does *NOT* change the fact that it will end up spinning if ->d_lock is held. All changes of dentry refcount *MUST* be under ->d_lock or be equivalent to such. You can do that directly if you are holding ->d_lock already, you can take it manually and do modification or you can use a function that does an equivalent of lock/modify/unlock. Additionally, dget() is only allowed if you are guaranteed to already hold a reference; it will go from 0 to 1, but it's really asking for trouble. dget_dlock() is allowed if dentry is not dead, i.e. if you know that it has not reached __dentry_kill() yet. Anything with refcount >= 0 after you grabbed ->d_lock is fine, since the very first thing __dentry_kill() does is setting refcount negative and does that before dropping ->d_lock. For the same reason anything found to be still hashed after you've grabbed ->d_lock is fine. Ditto for anything found on inode's aliases list (under ->i_lock and ->d_lock), for much the same reason. The same goes for any pointer that would've been removed by ->d_prune(). The same goes for anything with non-NULL ->d_inode (again, under ->d_lock). Or anything with non-empty list of children (since that'll guarantee positive refcount), etc. The real predicate is "had not been passed to __dentry_kill() yet"; the rest is a bunch of criteria sufficient for that. Shouldn't be all that many callers - or places that play with ->d_lock, for that matter. <checks> In #work.dcache2 at the moment: Checked simple_positive() under ->d_lock: arch/powerpc/platforms/cell/spufs/inode.c:155: dget_dlock(dentry); fs/autofs/expire.c:81: dget_dlock(child); fs/configfs/inode.c:211: dget_dlock(dentry); fs/libfs.c:120: found = dget_dlock(d); fs/libfs.c:410: found = dget_dlock(child); fs/libfs.c:498: child = dget_dlock(d); Check that dentry is positive under ->d_lock: fs/autofs/root.c:232: dget_dlock(expiring); Found in inode's list of aliases under ->i_lock and ->d_lock: fs/ceph/mds_client.c:4277: dn = dget_dlock(alias); fs/dcache.c:970: __dget_dlock(alias); fs/dcache.c:2719: __dget_dlock(alias); fs/ocfs2/dcache.c:165: dget_dlock(dentry); Found to be hashed under ->d_lock: fs/dcache.c:2361: dentry->d_lockref.count++; Check that refcount is greater than 0 under ->d_lock: fs/autofs/root.c:172: dget_dlock(active); Check that refcount is not negative under ->d_lock: fs/ceph/dir.c:1603: dget_dlock(dentry); ->d_parent of live dentry, refcount must be positive: fs/dcache.c:925: ret->d_lockref.count++; fs/dcache.c:2855: dentry->d_parent->d_lockref.count++; Found to be a mountpoint under ->d_lock; refcount must be positive: fs/dcache.c:1575: __dget_dlock(dentry); Caller must have already held a reference: fs/dcache.c:1721: __dget_dlock(parent); The worst of the entire bunch - associated ceph_dentry_info is found to be hashed under ->d_lock. That thing gets hashed by ceph_unlink(), with caller holding a reference to dentry and it is removed from hash (either by ceph_unlink() itself or by ceph_async_unlink_cb()) before the dentry reference gets dropped. Ceph is really gnarly around refcounting... fs/ceph/mds_client.c:864: found = dget_dlock(udentry); PS: Folks, please don't get confused by lockref; all it really does is an optimized variant of lock/modify/unlock on architectures that have reasonably cheap 64bit compare-and-swap and have sane spinlocks. Eqiuvalents of these primitives: lockref_get: lock count++ unlock lockref_get_not_zero: lock if (count > 0) count++ unlock return true else unlock return false lockref_put_not_zero: lock if (count > 1) count-- unlock return true else unlock return false lockref_put_or_lock: lock if count > 1 count-- unlock return true else return false // *WITHOUT* unlock lockref_get_not_dead: lock if (count >= 0) count++ unlock return true else unlock return false lockref_mark_dead: // must be called under lock count = -128 // negative; no reason -1 wouldn't do __lockref_is_dead: // ought to be used under lock, or it can count < 0 // go from false to true under you. // can be used as a check before bothering // with lock - if true, it's going to stay // true. There's also lockref_put_return, but that's really, really fastpath-only thing; unlike the rest of them it does not have a fallback and caller must provide one. About the only valid use is in fast_dput(); IMO that ought to be renamed to __lockref_put_return() to make trouble less likely. PPS: sparc64 almost certainly should go for these tricks; riscv probably would be fine too...
diff -- a/include/linux/dcache.h b/include/linux/dcache.h --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -301,12 +301,15 @@ extern char *dentry_path(const struct de /* Allocation counts.. */ /** - * dget, dget_dlock - get a reference to a dentry + * dget_dlock - get a reference to a dentry * @dentry: dentry to get a reference to * * Given a dentry or %NULL pointer increment the reference count - * if appropriate and return the dentry. A dentry will not be + * if appropriate and return the dentry. A dentry will not be * destroyed when it has references. + * + * The reference count increment in this function is not atomic. + * Consider dget() if atomicity is required. */ static inline struct dentry *dget_dlock(struct dentry *dentry) { @@ -315,6 +318,17 @@ static inline struct dentry *dget_dlock( return dentry; } +/** + * dget - get a reference to a dentry + * @dentry: dentry to get a reference to + * + * Given a dentry or %NULL pointer increment the reference count + * if appropriate and return the dentry. A dentry will not be + * destroyed when it has references. + * + * This function atomically increments the reference count. + * Consider dget_dlock() if atomicity is not required or manually managed. + */ static inline struct dentry *dget(struct dentry *dentry) { if (dentry)
Separate the kernel-doc notation for dget() and dget_dlock() to prevent a kernel-doc warning: include/linux/dcache.h:312: warning: expecting prototype for dget, dget_dlock(). Prototype was for dget_dlock() instead There have been several previous attempts to clean up these kernel-doc warnings: a. https://lore.kernel.org/lkml/20190327051717.23225-14-tobin@kernel.org/ b. https://lore.kernel.org/all/eec6afad98dddc2eef10a5c98a074a07aba50787.1657360984.git.mchehab@kernel.org/ c. https://lore.kernel.org/all/9d2676a83ebee327b97b82f3c2ab76a2e53756d1.1660829433.git.mchehab@kernel.org/ d. https://lore.kernel.org/all/Yvde4NryqBnZesTI@autolfshost/ e. https://lore.kernel.org/all/20230926163631.116405-1-n2h9z4@gmail.com/ Parts of this patch are from Anup's v3 patch [d]. Fixes: dc0474be3e27 ("fs: dcache rationalise dget variants") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-fsdevel@vger.kernel.org Cc: Tobin C. Harding <tobin@kernel.org> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Nik Bune <n2h9z4@gmail.com> Suggested-by: Anup K Parikh <parikhanupk.foss@gmail.com> --- include/linux/dcache.h | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)