| Message ID | 20240210100643.2207350-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dcache: rename d_genocide() | expand |
On Sat, Feb 10, 2024 at 12:06:43PM +0200, Amir Goldstein wrote: > I am not usually for PC culture and I know that you are on team > "freedom of speech" ;-), but IMO this one stood out for its high ratio > of bad taste vs. usefulness. > > The patch is based on your revert of "get rid of DCACHE_GENOCIDE". > I was hoping that you could queue my patch along with the revert. > > BTW, why was d_genocide() only dropping refcounts on the s_root tree > and not on the s_roots trees like shrink_dcache_for_umount()? > Is it because dentries on s_roots are not supposed to be hashed? Because secondary roots make no sense for "everything's in dcache" kind of filesystems, mostly. FWIW, I don't believe that cosmetic renaming is the right thing to do here. The real issue here is that those fs-pinned dentries are hard to distinguish. The rule is "if dentry on such filesystem is positive and hashed, that contributes 1 to its refcount". Unfortunately, that doesn't come with sane locking rules. If nothing else, I would rather have an explicit flag set along with bumping ->d_count on creation side and cleared along with dropping refcount on removal, both under ->d_lock. Another piece of ugliness is the remaining places that try to open-code simple_recursive_removal(); they get it wrong more often than not. Connected to the previous, since that's where those games with simple_unlink()/simple_rmdir() and associated fun with refcounts tend to happen. I'm trying to untangle that mess - on top of that revert, obviously. Interposing your patch in there is doable, of course, but it's not particularly useful, IMO, especially since the whole d_genocide() thing is quite likely to disappear, turning kill_litter_super() into an alias for kill_anon_super().
On Sun, Feb 11, 2024 at 1:27 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Sat, Feb 10, 2024 at 12:06:43PM +0200, Amir Goldstein wrote: > > > I am not usually for PC culture and I know that you are on team > > "freedom of speech" ;-), but IMO this one stood out for its high ratio > > of bad taste vs. usefulness. > > > > The patch is based on your revert of "get rid of DCACHE_GENOCIDE". > > I was hoping that you could queue my patch along with the revert. > > > > BTW, why was d_genocide() only dropping refcounts on the s_root tree > > and not on the s_roots trees like shrink_dcache_for_umount()? > > Is it because dentries on s_roots are not supposed to be hashed? > > Because secondary roots make no sense for "everything's in dcache" > kind of filesystems, mostly. > > FWIW, I don't believe that cosmetic renaming is the right thing to > do here. The real issue here is that those fs-pinned dentries are > hard to distinguish. The rule is "if dentry on such filesystem is > positive and hashed, that contributes 1 to its refcount". > > Unfortunately, that doesn't come with sane locking rules. > If nothing else, I would rather have an explicit flag set along > with bumping ->d_count on creation side and cleared along with > dropping refcount on removal, both under ->d_lock. > > Another piece of ugliness is the remaining places that try to > open-code simple_recursive_removal(); they get it wrong more > often than not. Connected to the previous, since that's where > those games with simple_unlink()/simple_rmdir() and associated > fun with refcounts tend to happen. > > I'm trying to untangle that mess - on top of that revert, obviously. That sounds perfect. > Interposing your patch in there is doable, of course, > but it's not particularly useful, IMO, especially since the Merging my rename patch is not the goal. Clarifying the code is. > whole d_genocide() thing is quite likely to disappear, turning > kill_litter_super() into an alias for kill_anon_super(). 2-in-1, getting rid of cruelty in the human and animal kingdom ;) Thanks, Amir.
On Sun, Feb 11, 2024 at 10:00:03AM +0200, Amir Goldstein wrote: > > whole d_genocide() thing is quite likely to disappear, turning > > kill_litter_super() into an alias for kill_anon_super(). > > 2-in-1, getting rid of cruelty in the human and animal kingdom ;) FWIW, "litter" in the above is in the sense of "trash", not "collection of animal offspring from the same birth"...
On Sun, Feb 11, 2024 at 8:44 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Sun, Feb 11, 2024 at 10:00:03AM +0200, Amir Goldstein wrote: > > > > whole d_genocide() thing is quite likely to disappear, turning > > > kill_litter_super() into an alias for kill_anon_super(). > > > > 2-in-1, getting rid of cruelty in the human and animal kingdom ;) > > FWIW, "litter" in the above is in the sense of "trash", not "collection > of animal offspring from the same birth"... Well either way, it is not clear by this name when the function should be used. The current documentation of when kill_litter_super() should be used is "use it for fs that set FS_LITTER flag, oh, BTW, there is no FS_LITTER flag": Documentation/filesystems/porting.rst: --- new file_system_type method - kill_sb(superblock). If you are converting an existing filesystem, set it according to ->fs_flags:: FS_REQUIRES_DEV - kill_block_super FS_LITTER - kill_litter_super neither - kill_anon_super FS_LITTER is gone - just remove it from fs_flags. --- If you are going to make kill_litter_super() an alias of kill_anon_super() I suggest going the extra mile and replacing the remaining 30 instances of kill_litter_super(). If the rules become straight forward for the default ->kill_sb(), then maybe we should even make ->kill_sb() optional and do: diff --git a/fs/super.c b/fs/super.c index d35e85295489..6200cac0e4f8 100644 --- a/fs/super.c +++ b/fs/super.c @@ -458,6 +458,18 @@ static void kill_super_notify(struct super_block *sb) super_wake(sb, SB_DEAD); } +static void kill_sb(struct super_block *s) +{ + struct file_system_type *fs = s->s_type; + + if (fs->kill_sb) + fs->kill_sb(s); + else if (fs->fs_flags & FS_REQUIRES_DEV) + kill_block_super(s); + else + kill_anon_super(s); +} + /** * deactivate_locked_super - drop an active reference to superblock * @s: superblock to deactivate @@ -474,7 +486,7 @@ void deactivate_locked_super(struct super_block *s) struct file_system_type *fs = s->s_type; if (atomic_dec_and_test(&s->s_active)) { shrinker_free(s->s_shrink); - fs->kill_sb(s); + kill_sb(s); kill_super_notify(s); -- Thanks, Amir.
On Mon, Feb 12, 2024 at 09:02:58AM +0200, Amir Goldstein wrote: > If you are going to make kill_litter_super() an alias of kill_anon_super() > I suggest going the extra mile and replacing the remaining 30 instances of > kill_litter_super(). > > If the rules become straight forward for the default ->kill_sb(), > then maybe we should even make ->kill_sb() optional and do: > > diff --git a/fs/super.c b/fs/super.c > index d35e85295489..6200cac0e4f8 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -458,6 +458,18 @@ static void kill_super_notify(struct super_block *sb) > super_wake(sb, SB_DEAD); > } > > +static void kill_sb(struct super_block *s) > +{ > + struct file_system_type *fs = s->s_type; > + > + if (fs->kill_sb) > + fs->kill_sb(s); > + else if (fs->fs_flags & FS_REQUIRES_DEV) > + kill_block_super(s); > + else > + kill_anon_super(s); > +} Bloody bad idea, IMO. Note that straight use of kill_anon_super() pretty much forces you into doing everything from ->put_super(). And that leads to rather clumsy failure exits in foo_fill_super(), since you *won't* get ->put_super() called unless you've got to setting ->s_root. Considering how easily the failure exits rot, I'd rather discourage that variant.
On Mon, Feb 12, 2024 at 08:09:26AM +0000, Al Viro wrote: > Bloody bad idea, IMO. Note that straight use of kill_anon_super() > pretty much forces you into doing everything from ->put_super(). > And that leads to rather clumsy failure exits in foo_fill_super(), > since you *won't* get ->put_super() called unless you've got to > setting ->s_root. > > Considering how easily the failure exits rot, I'd rather discourage > that variant. BTW, take a look at fs/ext2/super.c and compare the mess in failure exits in ext2_fill_super() with ext2_put_super(). See the amount of duplication? In case of ext2_fill_super() success eventual ->kill_sb() will call ->put_super() (from generic_shutdown_super(), from kill_block_super()). What happens in case of ext2_fill_super() failure? ->kill_sb() is called, but ->put_super() is only called if ->s_root is non-NULL (and at the very least it requires ->s_op to have been set). So in that case we have ext2_fill_super() manually undo the allocations, etc. it had managed to do, same as ext2_put_super() would've done. If that stuff gets lifted into ->kill_sb(), we get the bulk of ext2_put_super() moved into ext2_kill_super() (I wouldn't be surprised if ext2_put_super() completely disappeared, actually), with all those goto failed_mount<something> in ext2_fill_super() turning into plain return -E...
On Tue, Feb 13, 2024 at 6:42 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Mon, Feb 12, 2024 at 08:09:26AM +0000, Al Viro wrote: > > > Bloody bad idea, IMO. Note that straight use of kill_anon_super() > > pretty much forces you into doing everything from ->put_super(). > > And that leads to rather clumsy failure exits in foo_fill_super(), > > since you *won't* get ->put_super() called unless you've got to > > setting ->s_root. > > > > Considering how easily the failure exits rot, I'd rather discourage > > that variant. > > BTW, take a look at fs/ext2/super.c and compare the mess in failure > exits in ext2_fill_super() with ext2_put_super(). See the amount of > duplication? > > In case of ext2_fill_super() success eventual ->kill_sb() will call > ->put_super() (from generic_shutdown_super(), from kill_block_super()). > > What happens in case of ext2_fill_super() failure? ->kill_sb() is called, > but ->put_super() is only called if ->s_root is non-NULL (and at the very > least it requires ->s_op to have been set). So in that case we have > ext2_fill_super() manually undo the allocations, etc. it had managed to do, > same as ext2_put_super() would've done. > > If that stuff gets lifted into ->kill_sb(), we get the bulk of ext2_put_super() > moved into ext2_kill_super() (I wouldn't be surprised if ext2_put_super() > completely disappeared, actually), with all those goto failed_mount<something> > in ext2_fill_super() turning into plain return -E... That sounds good, but I am not sure what you are advocating for? What I wrote is that if kill_litter_super() becomes an alias of kill_anon_super(), then spraying kill_{anon,block}_super() automatically for new fs does not make much sense from API POV. It sounds like you are suggesting that the use of kill_{anon,block}_super() should be discouraged and that ext2 could be used to set an example. I did not understand if you are suggesting API changes to encourage customized ->kill_sb()? Thanks, Amir.
diff --git a/fs/dcache.c b/fs/dcache.c index 6ebccba33336..61ecc98c49a8 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -3054,24 +3054,27 @@ bool is_subdir(struct dentry *new_dentry, struct dentry *old_dentry) } EXPORT_SYMBOL(is_subdir); -static enum d_walk_ret d_genocide_kill(void *data, struct dentry *dentry) +/* make umount_check() happy before killing sb */ +static enum d_walk_ret dput_for_umount(void *data, struct dentry *dentry) { struct dentry *root = data; if (dentry != root) { if (d_unhashed(dentry) || !dentry->d_inode) return D_WALK_SKIP; - if (!(dentry->d_flags & DCACHE_GENOCIDE)) { - dentry->d_flags |= DCACHE_GENOCIDE; + if (!(dentry->d_flags & DCACHE_SB_DYING)) { + dentry->d_flags |= DCACHE_SB_DYING; dentry->d_lockref.count--; } } return D_WALK_CONTINUE; } -void d_genocide(struct dentry *parent) +/* drop last references before shrink_dcache_for_umount() */ +void dput_dcache_for_umount(struct super_block *sb) { - d_walk(parent, parent, d_genocide_kill); + if (sb->s_root) + d_walk(sb->s_root, sb->s_root, dput_for_umount); } void d_mark_tmpfile(struct file *file, struct inode *inode) diff --git a/fs/internal.h b/fs/internal.h index b67406435fc0..27bda8a3ff9d 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -215,10 +215,10 @@ extern char *simple_dname(struct dentry *, char *, int); extern void dput_to_list(struct dentry *, struct list_head *); extern void shrink_dentry_list(struct list_head *); extern void shrink_dcache_for_umount(struct super_block *); +extern void dput_dcache_for_umount(struct super_block *); extern struct dentry *__d_lookup(const struct dentry *, const struct qstr *); extern struct dentry *__d_lookup_rcu(const struct dentry *parent, const struct qstr *name, unsigned *seq); -extern void d_genocide(struct dentry *); /* * pipe.c diff --git a/fs/super.c b/fs/super.c index d35e85295489..42b3189fbe06 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1235,8 +1235,7 @@ EXPORT_SYMBOL(kill_anon_super); void kill_litter_super(struct super_block *sb) { - if (sb->s_root) - d_genocide(sb->s_root); + dput_dcache_for_umount(sb); kill_anon_super(sb); } EXPORT_SYMBOL(kill_litter_super); diff --git a/include/linux/dcache.h b/include/linux/dcache.h index d07cf2f1bb7d..0ce8543b64d7 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -173,7 +173,7 @@ struct dentry_operations { #define DCACHE_DONTCACHE BIT(7) /* Purge from memory on final dput() */ #define DCACHE_CANT_MOUNT BIT(8) -#define DCACHE_GENOCIDE BIT(9) +#define DCACHE_SB_DYING BIT(9) #define DCACHE_SHRINK_LIST BIT(10) #define DCACHE_OP_WEAK_REVALIDATE BIT(11)
Political context aside, using analogies from the real world in code is supposed to help us human programmers understand the code better. In the case of d_genocide(), not only is it a very dark analogy, but it's also a bad one, because d_genocide() does not actually kill any dentries. Rename it to dput_dcache_for_umount() and rename the DCACHE_GENOCIDE flag to DCACHE_SB_DYING. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Al, I am not usually for PC culture and I know that you are on team "freedom of speech" ;-), but IMO this one stood out for its high ratio of bad taste vs. usefulness. The patch is based on your revert of "get rid of DCACHE_GENOCIDE". I was hoping that you could queue my patch along with the revert. BTW, why was d_genocide() only dropping refcounts on the s_root tree and not on the s_roots trees like shrink_dcache_for_umount()? Is it because dentries on s_roots are not supposed to be hashed? Thanks, Amir. fs/dcache.c | 13 ++++++++----- fs/internal.h | 2 +- fs/super.c | 3 +-- include/linux/dcache.h | 2 +- 4 files changed, 11 insertions(+), 9 deletions(-)