| Message ID | 20250224212051.1756517-16-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/21] procfs: kill ->proc_dops | expand |
On Tue, 25 Feb 2025, Al Viro wrote: > makes simple_lookup() slightly cheaper there. I think the patch make sense because there is no point keeping negative dentries for these filesystems - and positive dentries get an extra refcount so DCACHE_DONTCACHE doesn't apply. But I don't see how this makes simple_lookup() cheaper. It means that if someone repeatedly looks up the same non-existent name then simple_lookup() will be called more often (because we didn't cache the result of the previous time) but otherwise I don't see the relevance to simple_lookup(). Am I missing something? Thanks, NeilBrown > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/hugetlbfs/inode.c | 1 + > fs/ramfs/inode.c | 1 + > ipc/mqueue.c | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index 0fc179a59830..205dd7562be1 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -1431,6 +1431,7 @@ hugetlbfs_fill_super(struct super_block *sb, struct fs_context *fc) > sb->s_blocksize_bits = huge_page_shift(ctx->hstate); > sb->s_magic = HUGETLBFS_MAGIC; > sb->s_op = &hugetlbfs_ops; > + sb->s_d_flags = DCACHE_DONTCACHE; > sb->s_time_gran = 1; > > /* > diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c > index 8006faaaf0ec..c4ee67870c4b 100644 > --- a/fs/ramfs/inode.c > +++ b/fs/ramfs/inode.c > @@ -269,6 +269,7 @@ static int ramfs_fill_super(struct super_block *sb, struct fs_context *fc) > sb->s_blocksize_bits = PAGE_SHIFT; > sb->s_magic = RAMFS_MAGIC; > sb->s_op = &ramfs_ops; > + sb->s_d_flags = DCACHE_DONTCACHE; > sb->s_time_gran = 1; > > inode = ramfs_get_inode(sb, NULL, S_IFDIR | fsi->mount_opts.mode, 0); > diff --git a/ipc/mqueue.c b/ipc/mqueue.c > index 35b4f8659904..dbd5c74eecb2 100644 > --- a/ipc/mqueue.c > +++ b/ipc/mqueue.c > @@ -411,6 +411,7 @@ static int mqueue_fill_super(struct super_block *sb, struct fs_context *fc) > sb->s_blocksize_bits = PAGE_SHIFT; > sb->s_magic = MQUEUE_MAGIC; > sb->s_op = &mqueue_super_ops; > + sb->s_d_flags = DCACHE_DONTCACHE; > > inode = mqueue_get_inode(sb, ns, S_IFDIR | S_ISVTX | S_IRWXUGO, NULL); > if (IS_ERR(inode)) > -- > 2.39.5 > >
On Wed, Feb 26, 2025 at 10:25:12AM +1100, NeilBrown wrote: > On Tue, 25 Feb 2025, Al Viro wrote: > > makes simple_lookup() slightly cheaper there. > > I think the patch make sense because there is no point keeping negative > dentries for these filesystems - and positive dentries get an extra > refcount so DCACHE_DONTCACHE doesn't apply. > > But I don't see how this makes simple_lookup() cheaper. It means that > if someone repeatedly looks up the same non-existent name then > simple_lookup() will be called more often (because we didn't cache the > result of the previous time) but otherwise I don't see the relevance to > simple_lookup(). Am I missing something? This: if (!(dentry->d_flags & DCACHE_DONTCACHE)) { spin_lock(&dentry->d_lock); dentry->d_flags |= DCACHE_DONTCACHE; spin_unlock(&dentry->d_lock); } IOW, no need to mark that sucker as "don't retain past the moment when its refcount drops to zero" - they'll all be marked that way since they'd been created. Note that we used to switch then to ->d_op that had ->d_delete always returning true. That had been replaced with setting DCACHE_DONTCACHE; that was an equivalent transformation. So retention rules have not changed; the only change here is that this flag doesn't need to be set.
On Wed, 26 Feb 2025, Al Viro wrote: > On Wed, Feb 26, 2025 at 10:25:12AM +1100, NeilBrown wrote: > > On Tue, 25 Feb 2025, Al Viro wrote: > > > makes simple_lookup() slightly cheaper there. > > > > I think the patch make sense because there is no point keeping negative > > dentries for these filesystems - and positive dentries get an extra > > refcount so DCACHE_DONTCACHE doesn't apply. > > > > But I don't see how this makes simple_lookup() cheaper. It means that > > if someone repeatedly looks up the same non-existent name then > > simple_lookup() will be called more often (because we didn't cache the > > result of the previous time) but otherwise I don't see the relevance to > > simple_lookup(). Am I missing something? > > This: > if (!(dentry->d_flags & DCACHE_DONTCACHE)) { > spin_lock(&dentry->d_lock); > dentry->d_flags |= DCACHE_DONTCACHE; > spin_unlock(&dentry->d_lock); > } Ah - right. Thanks. NeilBrown > > IOW, no need to mark that sucker as "don't retain past the moment when > its refcount drops to zero" - they'll all be marked that way since > they'd been created. > > Note that we used to switch then to ->d_op that had ->d_delete always > returning true. That had been replaced with setting DCACHE_DONTCACHE; > that was an equivalent transformation. So retention rules have not changed; > the only change here is that this flag doesn't need to be set. >
On Mon, Feb 24, 2025 at 09:20:46PM +0000, Al Viro wrote: > makes simple_lookup() slightly cheaper there. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- Reviewed-by: Christian Brauner <brauner@kernel.org> > fs/hugetlbfs/inode.c | 1 + > fs/ramfs/inode.c | 1 + > ipc/mqueue.c | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index 0fc179a59830..205dd7562be1 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -1431,6 +1431,7 @@ hugetlbfs_fill_super(struct super_block *sb, struct fs_context *fc) > sb->s_blocksize_bits = huge_page_shift(ctx->hstate); > sb->s_magic = HUGETLBFS_MAGIC; > sb->s_op = &hugetlbfs_ops; > + sb->s_d_flags = DCACHE_DONTCACHE; > sb->s_time_gran = 1; > > /* > diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c > index 8006faaaf0ec..c4ee67870c4b 100644 > --- a/fs/ramfs/inode.c > +++ b/fs/ramfs/inode.c > @@ -269,6 +269,7 @@ static int ramfs_fill_super(struct super_block *sb, struct fs_context *fc) > sb->s_blocksize_bits = PAGE_SHIFT; > sb->s_magic = RAMFS_MAGIC; > sb->s_op = &ramfs_ops; > + sb->s_d_flags = DCACHE_DONTCACHE; > sb->s_time_gran = 1; > > inode = ramfs_get_inode(sb, NULL, S_IFDIR | fsi->mount_opts.mode, 0); > diff --git a/ipc/mqueue.c b/ipc/mqueue.c > index 35b4f8659904..dbd5c74eecb2 100644 > --- a/ipc/mqueue.c > +++ b/ipc/mqueue.c > @@ -411,6 +411,7 @@ static int mqueue_fill_super(struct super_block *sb, struct fs_context *fc) > sb->s_blocksize_bits = PAGE_SHIFT; > sb->s_magic = MQUEUE_MAGIC; > sb->s_op = &mqueue_super_ops; > + sb->s_d_flags = DCACHE_DONTCACHE; > > inode = mqueue_get_inode(sb, ns, S_IFDIR | S_ISVTX | S_IRWXUGO, NULL); > if (IS_ERR(inode)) > -- > 2.39.5 >
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 0fc179a59830..205dd7562be1 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -1431,6 +1431,7 @@ hugetlbfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_blocksize_bits = huge_page_shift(ctx->hstate); sb->s_magic = HUGETLBFS_MAGIC; sb->s_op = &hugetlbfs_ops; + sb->s_d_flags = DCACHE_DONTCACHE; sb->s_time_gran = 1; /* diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c index 8006faaaf0ec..c4ee67870c4b 100644 --- a/fs/ramfs/inode.c +++ b/fs/ramfs/inode.c @@ -269,6 +269,7 @@ static int ramfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_blocksize_bits = PAGE_SHIFT; sb->s_magic = RAMFS_MAGIC; sb->s_op = &ramfs_ops; + sb->s_d_flags = DCACHE_DONTCACHE; sb->s_time_gran = 1; inode = ramfs_get_inode(sb, NULL, S_IFDIR | fsi->mount_opts.mode, 0); diff --git a/ipc/mqueue.c b/ipc/mqueue.c index 35b4f8659904..dbd5c74eecb2 100644 --- a/ipc/mqueue.c +++ b/ipc/mqueue.c @@ -411,6 +411,7 @@ static int mqueue_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_blocksize_bits = PAGE_SHIFT; sb->s_magic = MQUEUE_MAGIC; sb->s_op = &mqueue_super_ops; + sb->s_d_flags = DCACHE_DONTCACHE; inode = mqueue_get_inode(sb, ns, S_IFDIR | S_ISVTX | S_IRWXUGO, NULL); if (IS_ERR(inode))
makes simple_lookup() slightly cheaper there. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/hugetlbfs/inode.c | 1 + fs/ramfs/inode.c | 1 + ipc/mqueue.c | 1 + 3 files changed, 3 insertions(+)