| Message ID | 20250110024303.4157645-1-viro@zeniv.linux.org.uk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/20] make sure that DNAME_INLINE_LEN is a multiple of word size | expand |
On Thu, 9 Jan 2025 at 18:45, Al Viro <viro@zeniv.linux.org.uk> wrote: > > However, to reduce dentry_operations bloat, let's add one method instead - > ->d_want_unalias(alias, true) instead of ->d_unalias_trylock(alias) and > ->d_want_unalias(alias, false) instead of ->d_unalias_unlock(alias). Ugh. So of all the patches, this is the one that I hate. I absolutely detest interfaces with random true/false arguments, and when it is about locking, the "detest" becomes something even darker and more visceral. I think it would be a lot better as separate ops, considering that (a) we'll probably have only one or two actual users, so it's not like it complicates things on that side (b) we don't have *that* many "struct dentry_operations" structures: I think they are all statically generated constant structures (typically one or two per filesystem), so it's not like we're saving memory by merging those pointers into one. Please? Linus
On Thu, Jan 09, 2025 at 07:11:46PM -0800, Linus Torvalds wrote: > On Thu, 9 Jan 2025 at 18:45, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > However, to reduce dentry_operations bloat, let's add one method instead - > > ->d_want_unalias(alias, true) instead of ->d_unalias_trylock(alias) and > > ->d_want_unalias(alias, false) instead of ->d_unalias_unlock(alias). > > Ugh. > > So of all the patches, this is the one that I hate. > > I absolutely detest interfaces with random true/false arguments, and > when it is about locking, the "detest" becomes something even darker > and more visceral. > > I think it would be a lot better as separate ops, considering that > > (a) we'll probably have only one or two actual users, so it's not > like it complicates things on that side > > (b) we don't have *that* many "struct dentry_operations" structures: > I think they are all statically generated constant structures > (typically one or two per filesystem), so it's not like we're saving > memory by merging those pointers into one. ACK. > Please? Done and force-pushed; see below for updated variant of that commit commit 1f28d77e868e63a07ab50e7fe161fc366b2fb23b Author: Al Viro <viro@zeniv.linux.org.uk> Date: Sun Jan 5 21:33:17 2025 -0500 9p: fix ->rename_sem exclusion 9p wants to be able to build a path from given dentry to fs root and keep it valid over a blocking operation. ->s_vfs_rename_mutex would be a natural candidate, but there are places where we need that and where we have no way to tell if ->s_vfs_rename_mutex is already held deeper in callchain. Moreover, it's only held for cross-directory renames; name changes within the same directory happen without it. Solution: * have d_move() done in ->rename() rather than in its caller * maintain a 9p-private rwsem (per-filesystem) * hold it exclusive over the relevant part of ->rename() * hold it shared over the places where we want the path. That almost works. FS_RENAME_DOES_D_MOVE is enough to put all d_move() and d_exchange() calls under filesystem's control. However, there's also __d_unalias(), which isn't covered by any of that. If ->lookup() hits a directory inode with preexisting dentry elsewhere (due to e.g. rename done on server behind our back), d_splice_alias() called by ->lookup() will move/rename that alias. Add a couple of optional methods, so that __d_unalias() would do if alias->d_op->d_unalias_trylock != NULL if (!alias->d_op->d_unalias_trylock(alias)) fail (resulting in -ESTALE from lookup) __d_move(...) if alias->d_op->d_unalias_unlock != NULL alias->d_unalias_unlock(alias) where it currently does __d_move(). 9p instances do down_write_trylock() and up_write() of ->rename_mutex. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst index 146e7d8aa736..d20a32b77b60 100644 --- a/Documentation/filesystems/locking.rst +++ b/Documentation/filesystems/locking.rst @@ -31,6 +31,8 @@ prototypes:: struct vfsmount *(*d_automount)(struct path *path); int (*d_manage)(const struct path *, bool); struct dentry *(*d_real)(struct dentry *, enum d_real_type type); + bool (*d_unalias_trylock)(const struct dentry *); + void (*d_unalias_unlock)(const struct dentry *); locking rules: @@ -50,6 +52,8 @@ d_dname: no no no no d_automount: no no yes no d_manage: no no yes (ref-walk) maybe d_real no no yes no +d_unalias_trylock yes no no no +d_unalias_unlock yes no no no ================== =========== ======== ============== ======== inode_operations diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst index 7c352ebaae98..31eea688609a 100644 --- a/Documentation/filesystems/vfs.rst +++ b/Documentation/filesystems/vfs.rst @@ -1265,6 +1265,8 @@ defined: struct vfsmount *(*d_automount)(struct path *); int (*d_manage)(const struct path *, bool); struct dentry *(*d_real)(struct dentry *, enum d_real_type type); + bool (*d_unalias_trylock)(const struct dentry *); + void (*d_unalias_unlock)(const struct dentry *); }; ``d_revalidate`` @@ -1428,6 +1430,25 @@ defined: For non-regular files, the 'dentry' argument is returned. +``d_unalias_trylock`` + if present, will be called by d_splice_alias() before moving a + preexisting attached alias. Returning false prevents __d_move(), + making d_splice_alias() fail with -ESTALE. + + Rationale: setting FS_RENAME_DOES_D_MOVE will prevent d_move() + and d_exchange() calls from the outside of filesystem methods; + however, it does not guarantee that attached dentries won't + be renamed or moved by d_splice_alias() finding a preexisting + alias for a directory inode. Normally we would not care; + however, something that wants to stabilize the entire path to + root over a blocking operation might need that. See 9p for one + (and hopefully only) example. + +``d_unalias_unlock`` + should be paired with ``d_unalias_trylock``; that one is called after + __d_move() call in __d_unalias(). + + Each dentry has a pointer to its parent dentry, as well as a hash list of child dentries. Child dentries are basically like files in a directory. diff --git a/fs/9p/v9fs.h b/fs/9p/v9fs.h index 698c43dd5dc8..f28bc763847a 100644 --- a/fs/9p/v9fs.h +++ b/fs/9p/v9fs.h @@ -202,7 +202,7 @@ static inline struct v9fs_session_info *v9fs_inode2v9ses(struct inode *inode) return inode->i_sb->s_fs_info; } -static inline struct v9fs_session_info *v9fs_dentry2v9ses(struct dentry *dentry) +static inline struct v9fs_session_info *v9fs_dentry2v9ses(const struct dentry *dentry) { return dentry->d_sb->s_fs_info; } diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c index 872c1abe3295..5061f192eafd 100644 --- a/fs/9p/vfs_dentry.c +++ b/fs/9p/vfs_dentry.c @@ -105,14 +105,30 @@ static int v9fs_lookup_revalidate(struct inode *dir, const struct qstr *name, return __v9fs_lookup_revalidate(dentry, flags); } +static bool v9fs_dentry_unalias_trylock(const struct dentry *dentry) +{ + struct v9fs_session_info *v9ses = v9fs_dentry2v9ses(dentry); + return down_write_trylock(&v9ses->rename_sem); +} + +static void v9fs_dentry_unalias_unlock(const struct dentry *dentry) +{ + struct v9fs_session_info *v9ses = v9fs_dentry2v9ses(dentry); + up_write(&v9ses->rename_sem); +} + const struct dentry_operations v9fs_cached_dentry_operations = { .d_revalidate = v9fs_lookup_revalidate, .d_weak_revalidate = __v9fs_lookup_revalidate, .d_delete = v9fs_cached_dentry_delete, .d_release = v9fs_dentry_release, + .d_unalias_trylock = v9fs_dentry_unalias_trylock, + .d_unalias_unlock = v9fs_dentry_unalias_unlock, }; const struct dentry_operations v9fs_dentry_operations = { .d_delete = always_delete_dentry, .d_release = v9fs_dentry_release, + .d_unalias_trylock = v9fs_dentry_unalias_trylock, + .d_unalias_unlock = v9fs_dentry_unalias_unlock, }; diff --git a/fs/dcache.c b/fs/dcache.c index 7d42ca367522..2ac614fc8bba 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -2961,7 +2961,12 @@ static int __d_unalias(struct dentry *dentry, struct dentry *alias) goto out_err; m2 = &alias->d_parent->d_inode->i_rwsem; out_unalias: + if (alias->d_op->d_unalias_trylock && + !alias->d_op->d_unalias_trylock(alias)) + goto out_err; __d_move(alias, dentry, false); + if (alias->d_op->d_unalias_unlock) + alias->d_op->d_unalias_unlock(alias); ret = 0; out_err: if (m2) diff --git a/include/linux/dcache.h b/include/linux/dcache.h index 4a6bdadf2f29..9a1a30857763 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -159,6 +159,8 @@ struct dentry_operations { struct vfsmount *(*d_automount)(struct path *); int (*d_manage)(const struct path *, bool); struct dentry *(*d_real)(struct dentry *, enum d_real_type type); + bool (*d_unalias_trylock)(const struct dentry *); + void (*d_unalias_unlock)(const struct dentry *); } ____cacheline_aligned; /*
Al Viro <viro@zeniv.linux.org.uk> wrote: > struct external_name { > - struct { > - atomic_t count; // ->count and ->head can't be combined > - struct rcu_head head; // see take_dentry_name_snapshot() > - } u; > + atomic_t count; // ->count and ->head can't be combined > + struct rcu_head head; // see take_dentry_name_snapshot() > unsigned char name[]; > }; This gets you a 4-byte hole between count and head on a 64-bit system. Did you want to flip the order of count and head? David
On Fri 10-01-25 02:42:48, Al Viro wrote: > ... rather than open-coding them. As a bonus, that avoids the pointless > work with extra allocations, etc. for long names. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Nice! Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/fast_commit.c | 29 +++++------------------------ > fs/ext4/fast_commit.h | 3 +-- > 2 files changed, 6 insertions(+), 26 deletions(-) > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c > index 26c4fc37edcf..da4263a14a20 100644 > --- a/fs/ext4/fast_commit.c > +++ b/fs/ext4/fast_commit.c > @@ -322,9 +322,7 @@ void ext4_fc_del(struct inode *inode) > WARN_ON(!list_empty(&ei->i_fc_dilist)); > spin_unlock(&sbi->s_fc_lock); > > - if (fc_dentry->fcd_name.name && > - fc_dentry->fcd_name.len > DNAME_INLINE_LEN) > - kfree(fc_dentry->fcd_name.name); > + release_dentry_name_snapshot(&fc_dentry->fcd_name); > kmem_cache_free(ext4_fc_dentry_cachep, fc_dentry); > > return; > @@ -449,22 +447,7 @@ static int __track_dentry_update(handle_t *handle, struct inode *inode, > node->fcd_op = dentry_update->op; > node->fcd_parent = dir->i_ino; > node->fcd_ino = inode->i_ino; > - if (dentry->d_name.len > DNAME_INLINE_LEN) { > - node->fcd_name.name = kmalloc(dentry->d_name.len, GFP_NOFS); > - if (!node->fcd_name.name) { > - kmem_cache_free(ext4_fc_dentry_cachep, node); > - ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle); > - mutex_lock(&ei->i_fc_lock); > - return -ENOMEM; > - } > - memcpy((u8 *)node->fcd_name.name, dentry->d_name.name, > - dentry->d_name.len); > - } else { > - memcpy(node->fcd_iname, dentry->d_name.name, > - dentry->d_name.len); > - node->fcd_name.name = node->fcd_iname; > - } > - node->fcd_name.len = dentry->d_name.len; > + take_dentry_name_snapshot(&node->fcd_name, dentry); > INIT_LIST_HEAD(&node->fcd_dilist); > spin_lock(&sbi->s_fc_lock); > if (sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING || > @@ -832,7 +815,7 @@ static bool ext4_fc_add_dentry_tlv(struct super_block *sb, u32 *crc, > { > struct ext4_fc_dentry_info fcd; > struct ext4_fc_tl tl; > - int dlen = fc_dentry->fcd_name.len; > + int dlen = fc_dentry->fcd_name.name.len; > u8 *dst = ext4_fc_reserve_space(sb, > EXT4_FC_TAG_BASE_LEN + sizeof(fcd) + dlen, crc); > > @@ -847,7 +830,7 @@ static bool ext4_fc_add_dentry_tlv(struct super_block *sb, u32 *crc, > dst += EXT4_FC_TAG_BASE_LEN; > memcpy(dst, &fcd, sizeof(fcd)); > dst += sizeof(fcd); > - memcpy(dst, fc_dentry->fcd_name.name, dlen); > + memcpy(dst, fc_dentry->fcd_name.name.name, dlen); > > return true; > } > @@ -1328,9 +1311,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) > list_del_init(&fc_dentry->fcd_dilist); > spin_unlock(&sbi->s_fc_lock); > > - if (fc_dentry->fcd_name.name && > - fc_dentry->fcd_name.len > DNAME_INLINE_LEN) > - kfree(fc_dentry->fcd_name.name); > + release_dentry_name_snapshot(&fc_dentry->fcd_name); > kmem_cache_free(ext4_fc_dentry_cachep, fc_dentry); > spin_lock(&sbi->s_fc_lock); > } > diff --git a/fs/ext4/fast_commit.h b/fs/ext4/fast_commit.h > index 2fadb2c4780c..3bd534e4dbbf 100644 > --- a/fs/ext4/fast_commit.h > +++ b/fs/ext4/fast_commit.h > @@ -109,8 +109,7 @@ struct ext4_fc_dentry_update { > int fcd_op; /* Type of update create / unlink / link */ > int fcd_parent; /* Parent inode number */ > int fcd_ino; /* Inode number */ > - struct qstr fcd_name; /* Dirent name */ > - unsigned char fcd_iname[DNAME_INLINE_LEN]; /* Dirent name string */ > + struct name_snapshot fcd_name; /* Dirent name */ > struct list_head fcd_list; > struct list_head fcd_dilist; > }; > -- > 2.39.5 >
On Fri 10-01-25 02:42:44, Al Viro wrote: > ... calling the number of words DNAME_INLINE_WORDS. > > The next step will be to have a structure to hold inline name arrays > (both in dentry and in name_snapshot) and use that to alias the > existing arrays of unsigned char there. That will allow both > full-structure copies and convenient word-by-word accesses. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/dcache.c | 4 +--- > include/linux/dcache.h | 8 +++++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/dcache.c b/fs/dcache.c > index b4d5e9e1e43d..ea0f0bea511b 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -2748,9 +2748,7 @@ static void swap_names(struct dentry *dentry, struct dentry *target) > /* > * Both are internal. > */ > - unsigned int i; > - BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long))); > - for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) { > + for (int i = 0; i < DNAME_INLINE_WORDS; i++) { > swap(((long *) &dentry->d_iname)[i], > ((long *) &target->d_iname)[i]); > } > diff --git a/include/linux/dcache.h b/include/linux/dcache.h > index bff956f7b2b9..42dd89beaf4e 100644 > --- a/include/linux/dcache.h > +++ b/include/linux/dcache.h > @@ -68,15 +68,17 @@ extern const struct qstr dotdot_name; > * large memory footprint increase). > */ > #ifdef CONFIG_64BIT > -# define DNAME_INLINE_LEN 40 /* 192 bytes */ > +# define DNAME_INLINE_WORDS 5 /* 192 bytes */ > #else > # ifdef CONFIG_SMP > -# define DNAME_INLINE_LEN 36 /* 128 bytes */ > +# define DNAME_INLINE_WORDS 9 /* 128 bytes */ > # else > -# define DNAME_INLINE_LEN 44 /* 128 bytes */ > +# define DNAME_INLINE_WORDS 11 /* 128 bytes */ > # endif > #endif > > +#define DNAME_INLINE_LEN (DNAME_INLINE_WORDS*sizeof(unsigned long)) > + > #define d_lock d_lockref.lock > > struct dentry { > -- > 2.39.5 >
On Fri, Jan 10, 2025 at 07:34:11AM +0000, David Howells wrote: > Al Viro <viro@zeniv.linux.org.uk> wrote: > > > struct external_name { > > - struct { > > - atomic_t count; // ->count and ->head can't be combined > > - struct rcu_head head; // see take_dentry_name_snapshot() > > - } u; > > + atomic_t count; // ->count and ->head can't be combined > > + struct rcu_head head; // see take_dentry_name_snapshot() > > unsigned char name[]; > > }; > > This gets you a 4-byte hole between count and head on a 64-bit system. Did > you want to flip the order of count and head? Umm... Could do, but that probably wouldn't be that much of a win - we use those for names >= 40 characters long, and currently the size is 25 + len bytes. And it's kmalloc'ed, so anything in range 40...71 goes into kmalloc-96. Reordering those would have 40..43 land in kmalloc-64, leaving the rest as-is. Might as well...
On Fri, 2025-01-10 at 02:42 +0000, Al Viro wrote: > No need to mess with the boilerplate for obtaining what we already > have. Note that ceph is one of the "will want a path from filesystem > root if we want to talk to server" cases, so the name of the last > component is of little use - it is passed to fscrypt_d_revalidate() > and it's used to deal with (also crypt-related) case in request > marshalling, when encrypted name turns out to be too long. The > former > is not a problem, but the latter is racy; that part will be handled > in the next commit. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/ceph/dir.c | 22 ++++------------------ > 1 file changed, 4 insertions(+), 18 deletions(-) > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index c4c71c24221b..dc5f55bebad7 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1940,30 +1940,19 @@ static int dir_lease_is_valid(struct inode > *dir, struct dentry *dentry, > /* > * Check if cached dentry can be trusted. > */ > -static int ceph_d_revalidate(struct inode *parent_dir, const struct > qstr *name, > +static int ceph_d_revalidate(struct inode *dir, const struct qstr > *name, > struct dentry *dentry, unsigned int > flags) > { > struct ceph_mds_client *mdsc = ceph_sb_to_fs_client(dentry- > >d_sb)->mdsc; > struct ceph_client *cl = mdsc->fsc->client; > int valid = 0; > - struct dentry *parent; > - struct inode *dir, *inode; > + struct inode *inode; > > - valid = fscrypt_d_revalidate(parent_dir, name, dentry, > flags); > + valid = fscrypt_d_revalidate(dir, name, dentry, flags); > if (valid <= 0) > return valid; > > - if (flags & LOOKUP_RCU) { > - parent = READ_ONCE(dentry->d_parent); > - dir = d_inode_rcu(parent); > - if (!dir) > - return -ECHILD; > - inode = d_inode_rcu(dentry); > - } else { > - parent = dget_parent(dentry); > - dir = d_inode(parent); > - inode = d_inode(dentry); > - } > + inode = d_inode_rcu(dentry); > > doutc(cl, "%p '%pd' inode %p offset 0x%llx nokey %d\n", > dentry, dentry, inode, ceph_dentry(dentry)->offset, > @@ -2039,9 +2028,6 @@ static int ceph_d_revalidate(struct inode > *parent_dir, const struct qstr *name, > doutc(cl, "%p '%pd' %s\n", dentry, dentry, valid ? "valid" : > "invalid"); > if (!valid) > ceph_dir_clear_complete(dir); > - > - if (!(flags & LOOKUP_RCU)) > - dput(parent); > return valid; > } > Looks much better now. Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> Thanks, Slava.
diff --git a/fs/dcache.c b/fs/dcache.c index b4d5e9e1e43d..ea0f0bea511b 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -2748,9 +2748,7 @@ static void swap_names(struct dentry *dentry, struct dentry *target) /* * Both are internal. */ - unsigned int i; - BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long))); - for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) { + for (int i = 0; i < DNAME_INLINE_WORDS; i++) { swap(((long *) &dentry->d_iname)[i], ((long *) &target->d_iname)[i]); } diff --git a/include/linux/dcache.h b/include/linux/dcache.h index bff956f7b2b9..42dd89beaf4e 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -68,15 +68,17 @@ extern const struct qstr dotdot_name; * large memory footprint increase). */ #ifdef CONFIG_64BIT -# define DNAME_INLINE_LEN 40 /* 192 bytes */ +# define DNAME_INLINE_WORDS 5 /* 192 bytes */ #else # ifdef CONFIG_SMP -# define DNAME_INLINE_LEN 36 /* 128 bytes */ +# define DNAME_INLINE_WORDS 9 /* 128 bytes */ # else -# define DNAME_INLINE_LEN 44 /* 128 bytes */ +# define DNAME_INLINE_WORDS 11 /* 128 bytes */ # endif #endif +#define DNAME_INLINE_LEN (DNAME_INLINE_WORDS*sizeof(unsigned long)) + #define d_lock d_lockref.lock struct dentry {
... calling the number of words DNAME_INLINE_WORDS. The next step will be to have a structure to hold inline name arrays (both in dentry and in name_snapshot) and use that to alias the existing arrays of unsigned char there. That will allow both full-structure copies and convenient word-by-word accesses. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/dcache.c | 4 +--- include/linux/dcache.h | 8 +++++--- 2 files changed, 6 insertions(+), 6 deletions(-)