[PATCH] btrfs: use simple_dir_inode_operations for placeholder subvolume directory

[PATCH] btrfs: use simple_dir_inode_operations for placeholder subvolume directory

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

When you snapshot a subvolume containing a subvolume, you get a
placeholder directory where the subvolume would be. These directories
have their own btrfs_dir_ro_inode_operations.

Al pointed out [1] that these directories can use simple_lookup()
instead of btrfs_lookup(), as they are always empty. Furthermore, they
can use the default generic_permission() instead of btrfs_permission();
the additional checks in the latter don't matter because we can't write
to the directory anyways. Finally, they can use the default
generic_update_time() instead of btrfs_update_time(), as the inode
doesn't exist on disk and doesn't need any special handling.

All together, this means that we can get rid of
btrfs_dir_ro_inode_operations and use simple_dir_inode_operations
instead.

1: https://lore.kernel.org/linux-btrfs/20190929052934.GY26530@ZenIV.linux.org.uk/

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/inode.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fec2a78de037..9dc84a88ef0c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -64,7 +64,6 @@ struct btrfs_dio_data {
 
 static const struct inode_operations btrfs_dir_inode_operations;
 static const struct inode_operations btrfs_symlink_inode_operations;
-static const struct inode_operations btrfs_dir_ro_inode_operations;
 static const struct inode_operations btrfs_special_inode_operations;
 static const struct inode_operations btrfs_file_inode_operations;
 static const struct address_space_operations btrfs_aops;
@@ -5846,7 +5845,7 @@ static struct inode *new_simple_dir(struct super_block *s,
 	set_bit(BTRFS_INODE_DUMMY, &BTRFS_I(inode)->runtime_flags);
 
 	inode->i_ino = BTRFS_EMPTY_SUBVOL_DIR_OBJECTID;
-	inode->i_op = &btrfs_dir_ro_inode_operations;
+	inode->i_op = &simple_dir_inode_operations;
 	inode->i_opflags &= ~IOP_XATTR;
 	inode->i_fop = &simple_dir_operations;
 	inode->i_mode = S_IFDIR | S_IRUGO | S_IWUSR | S_IXUGO;
@@ -11001,11 +11000,6 @@ static const struct inode_operations btrfs_dir_inode_operations = {
 	.update_time	= btrfs_update_time,
 	.tmpfile        = btrfs_tmpfile,
 };
-static const struct inode_operations btrfs_dir_ro_inode_operations = {
-	.lookup		= btrfs_lookup,
-	.permission	= btrfs_permission,
-	.update_time	= btrfs_update_time,
-};
 
 static const struct file_operations btrfs_dir_file_operations = {
 	.llseek		= generic_file_llseek,
-- 
2.24.0

Re: [PATCH] btrfs: use simple_dir_inode_operations for placeholder subvolume directory

[Josef Bacik]

On Thu, Dec 05, 2019 at 10:36:04AM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> When you snapshot a subvolume containing a subvolume, you get a
> placeholder directory where the subvolume would be. These directories
> have their own btrfs_dir_ro_inode_operations.
> 
> Al pointed out [1] that these directories can use simple_lookup()
> instead of btrfs_lookup(), as they are always empty. Furthermore, they
> can use the default generic_permission() instead of btrfs_permission();
> the additional checks in the latter don't matter because we can't write
> to the directory anyways. Finally, they can use the default
> generic_update_time() instead of btrfs_update_time(), as the inode
> doesn't exist on disk and doesn't need any special handling.
> 
> All together, this means that we can get rid of
> btrfs_dir_ro_inode_operations and use simple_dir_inode_operations
> instead.
> 
> 1: https://lore.kernel.org/linux-btrfs/20190929052934.GY26530@ZenIV.linux.org.uk/
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs: use simple_dir_inode_operations for placeholder subvolume directory

[David Sterba]

On Thu, Dec 05, 2019 at 10:36:04AM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> When you snapshot a subvolume containing a subvolume, you get a
> placeholder directory where the subvolume would be. These directories
> have their own btrfs_dir_ro_inode_operations.
> 
> Al pointed out [1] that these directories can use simple_lookup()
> instead of btrfs_lookup(), as they are always empty. Furthermore, they
> can use the default generic_permission() instead of btrfs_permission();
> the additional checks in the latter don't matter because we can't write
> to the directory anyways. Finally, they can use the default
> generic_update_time() instead of btrfs_update_time(), as the inode
> doesn't exist on disk and doesn't need any special handling.
> 
> All together, this means that we can get rid of
> btrfs_dir_ro_inode_operations and use simple_dir_inode_operations
> instead.
> 
> 1: https://lore.kernel.org/linux-btrfs/20190929052934.GY26530@ZenIV.linux.org.uk/
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Added to misc-next, with a comment why we use the simple ops, thanks.