diff mbox series

[v2,02/11] btrfs: export the expected file extent through can_nocow_extent()

Message ID e351482ededb22c1fb81eeed217ae8e34e8e1427.1714707707.git.wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: extent-map: unify the members with btrfs_ordered_extent | expand

Commit Message

Qu Wenruo May 3, 2024, 6:01 a.m. UTC 
Currently function can_nocow_extent() only returns members needed for
extent_map.

However since we will soon change the extent_map structure to be more
like btrfs_file_extent_item, we want to expose the expected file extent
caused by the NOCOW write for future usage.

This would introduce a new structure, btrfs_file_extent, to be a more
memory-access-friendly representation of btrfs_file_extent_item.
And use that structure to expose the expected file extent caused by the
NOCOW write.

For now there is no user of the new structure yet.

Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/btrfs_inode.h | 20 +++++++++++++++++++-
 fs/btrfs/file.c        |  2 +-
 fs/btrfs/inode.c       | 22 +++++++++++++++++++---
 3 files changed, 39 insertions(+), 5 deletions(-)

Comments

Filipe Manana May 9, 2024, 4:22 p.m. UTC | #1 
On Fri, May 3, 2024 at 7:02 AM Qu Wenruo <wqu@suse.com> wrote:
>
> Currently function can_nocow_extent() only returns members needed for
> extent_map.
>
> However since we will soon change the extent_map structure to be more
> like btrfs_file_extent_item, we want to expose the expected file extent
> caused by the NOCOW write for future usage.
>
> This would introduce a new structure, btrfs_file_extent, to be a more

"would introduce" -> introduces

> memory-access-friendly representation of btrfs_file_extent_item.
> And use that structure to expose the expected file extent caused by the
> NOCOW write.
>
> For now there is no user of the new structure yet.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/btrfs_inode.h | 20 +++++++++++++++++++-
>  fs/btrfs/file.c        |  2 +-
>  fs/btrfs/inode.c       | 22 +++++++++++++++++++---
>  3 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index de918d89a582..18678762615a 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -443,9 +443,27 @@ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
>                             u32 pgoff, u8 *csum, const u8 * const csum_expected);
>  bool btrfs_data_csum_ok(struct btrfs_bio *bbio, struct btrfs_device *dev,
>                         u32 bio_offset, struct bio_vec *bv);
> +
> +/*
> + * A more access-friendly representation of btrfs_file_extent_item.
> + *
> + * Unused members are excluded.

A bit confusing this, you want to say this is a subset of
btrfs_file_extent_item.
So just say something like:

This represents details about the target file extent item of a write operation.

> + */
> +struct btrfs_file_extent {
> +       u64 disk_bytenr;
> +       u64 disk_num_bytes;
> +
> +       u64 num_bytes;
> +       u64 ram_bytes;
> +       u64 offset;
> +
> +       u8 compression;

The blank lines between the members seem kind of ad-hoc, redundant to
me - i.e. no logical grouping.
Maybe I missed something?

> +};
> +
>  noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
>                               u64 *orig_start, u64 *orig_block_len,
> -                             u64 *ram_bytes, bool nowait, bool strict);
> +                             u64 *ram_bytes, struct btrfs_file_extent *file_extent,
> +                             bool nowait, bool strict);
>
>  void btrfs_del_delalloc_inode(struct btrfs_inode *inode);
>  struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry);
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index d3cbd161cd90..63a13a4cace0 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1104,7 +1104,7 @@ int btrfs_check_nocow_lock(struct btrfs_inode *inode, loff_t pos,
>                                                    &cached_state);
>         }
>         ret = can_nocow_extent(&inode->vfs_inode, lockstart, &num_bytes,
> -                       NULL, NULL, NULL, nowait, false);
> +                       NULL, NULL, NULL, NULL, nowait, false);
>         if (ret <= 0)
>                 btrfs_drew_write_unlock(&root->snapshot_lock);
>         else
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0e5d4517af0e..2815b72f2d85 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1857,6 +1857,9 @@ struct can_nocow_file_extent_args {
>         u64 extent_offset;
>         /* Number of bytes that can be written to in NOCOW mode. */
>         u64 num_bytes;
> +
> +       /* The expected file extent for the NOCOW write. */
> +       struct btrfs_file_extent file_extent;
>  };
>
>  /*
> @@ -1921,6 +1924,12 @@ static int can_nocow_file_extent(struct btrfs_path *path,
>
>         extent_end = btrfs_file_extent_end(path);
>
> +       args->file_extent.disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
> +       args->file_extent.disk_num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
> +       args->file_extent.ram_bytes = btrfs_file_extent_ram_bytes(leaf, fi);
> +       args->file_extent.offset = btrfs_file_extent_offset(leaf, fi);
> +       args->file_extent.compression = btrfs_file_extent_compression(leaf, fi);
> +
>         /*
>          * The following checks can be expensive, as they need to take other
>          * locks and do btree or rbtree searches, so release the path to avoid
> @@ -1955,6 +1964,9 @@ static int can_nocow_file_extent(struct btrfs_path *path,
>         args->disk_bytenr += args->start - key->offset;
>         args->num_bytes = min(args->end + 1, extent_end) - args->start;
>
> +       args->file_extent.num_bytes = args->num_bytes;
> +       args->file_extent.offset += args->start - key->offset;
> +
>         /*
>          * Force COW if csums exist in the range. This ensures that csums for a
>          * given extent are either valid or do not exist.
> @@ -7099,7 +7111,8 @@ static bool btrfs_extent_readonly(struct btrfs_fs_info *fs_info, u64 bytenr)
>   */
>  noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
>                               u64 *orig_start, u64 *orig_block_len,
> -                             u64 *ram_bytes, bool nowait, bool strict)
> +                             u64 *ram_bytes, struct btrfs_file_extent *file_extent,
> +                             bool nowait, bool strict)
>  {
>         struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
>         struct can_nocow_file_extent_args nocow_args = { 0 };
> @@ -7188,6 +7201,9 @@ noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
>                 *orig_start = key.offset - nocow_args.extent_offset;
>         if (orig_block_len)
>                 *orig_block_len = nocow_args.disk_num_bytes;
> +       if (file_extent)
> +               memcpy(file_extent, &nocow_args.file_extent,
> +                      sizeof(struct btrfs_file_extent));

Can use instead sizeof(*file_extent), and it will allow to have
everything in the same line without being over 80 characters.

The rest looks fine, but I have to read the rest of the patchset to
see how this is actually used.

Thanks.

>
>         *len = nocow_args.num_bytes;
>         ret = 1;
> @@ -7407,7 +7423,7 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
>                 block_start = em->block_start + (start - em->start);
>
>                 if (can_nocow_extent(inode, start, &len, &orig_start,
> -                                    &orig_block_len, &ram_bytes, false, false) == 1) {
> +                                    &orig_block_len, &ram_bytes, NULL, false, false) == 1) {
>                         bg = btrfs_inc_nocow_writers(fs_info, block_start);
>                         if (bg)
>                                 can_nocow = true;
> @@ -10660,7 +10676,7 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
>                 free_extent_map(em);
>                 em = NULL;
>
> -               ret = can_nocow_extent(inode, start, &len, NULL, NULL, NULL, false, true);
> +               ret = can_nocow_extent(inode, start, &len, NULL, NULL, NULL, NULL, false, true);
>                 if (ret < 0) {
>                         goto out;
>                 } else if (ret) {
> --
> 2.45.0
>
>
Qu Wenruo May 9, 2024, 9:55 p.m. UTC | #2 
在 2024/5/10 01:52, Filipe Manana 写道:
[...]
>> + */
>> +struct btrfs_file_extent {
>> +       u64 disk_bytenr;
>> +       u64 disk_num_bytes;
>> +
>> +       u64 num_bytes;
>> +       u64 ram_bytes;
>> +       u64 offset;
>> +
>> +       u8 compression;
>
> The blank lines between the members seem kind of ad-hoc, redundant to
> me - i.e. no logical grouping.
> Maybe I missed something?

This is to separate the members so that, the disk_* ones are only
referring to a data extent uniquely.

The ram_bytes/num_bytes/offset are the ones to specify the referred
range inside the uncompressed extent.
Finally a u8 because it's not u64.

But for sure, I can remove all the new lines, because the grouping is
not that strong.

Thanks,
Qu
diff mbox series

Patch

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index de918d89a582..18678762615a 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -443,9 +443,27 @@  int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
 			    u32 pgoff, u8 *csum, const u8 * const csum_expected);
 bool btrfs_data_csum_ok(struct btrfs_bio *bbio, struct btrfs_device *dev,
 			u32 bio_offset, struct bio_vec *bv);
+
+/*
+ * A more access-friendly representation of btrfs_file_extent_item.
+ *
+ * Unused members are excluded.
+ */
+struct btrfs_file_extent {
+	u64 disk_bytenr;
+	u64 disk_num_bytes;
+
+	u64 num_bytes;
+	u64 ram_bytes;
+	u64 offset;
+
+	u8 compression;
+};
+
 noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
 			      u64 *orig_start, u64 *orig_block_len,
-			      u64 *ram_bytes, bool nowait, bool strict);
+			      u64 *ram_bytes, struct btrfs_file_extent *file_extent,
+			      bool nowait, bool strict);
 
 void btrfs_del_delalloc_inode(struct btrfs_inode *inode);
 struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry);
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index d3cbd161cd90..63a13a4cace0 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1104,7 +1104,7 @@  int btrfs_check_nocow_lock(struct btrfs_inode *inode, loff_t pos,
 						   &cached_state);
 	}
 	ret = can_nocow_extent(&inode->vfs_inode, lockstart, &num_bytes,
-			NULL, NULL, NULL, nowait, false);
+			NULL, NULL, NULL, NULL, nowait, false);
 	if (ret <= 0)
 		btrfs_drew_write_unlock(&root->snapshot_lock);
 	else
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0e5d4517af0e..2815b72f2d85 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1857,6 +1857,9 @@  struct can_nocow_file_extent_args {
 	u64 extent_offset;
 	/* Number of bytes that can be written to in NOCOW mode. */
 	u64 num_bytes;
+
+	/* The expected file extent for the NOCOW write. */
+	struct btrfs_file_extent file_extent;
 };
 
 /*
@@ -1921,6 +1924,12 @@  static int can_nocow_file_extent(struct btrfs_path *path,
 
 	extent_end = btrfs_file_extent_end(path);
 
+	args->file_extent.disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
+	args->file_extent.disk_num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
+	args->file_extent.ram_bytes = btrfs_file_extent_ram_bytes(leaf, fi);
+	args->file_extent.offset = btrfs_file_extent_offset(leaf, fi);
+	args->file_extent.compression = btrfs_file_extent_compression(leaf, fi);
+
 	/*
 	 * The following checks can be expensive, as they need to take other
 	 * locks and do btree or rbtree searches, so release the path to avoid
@@ -1955,6 +1964,9 @@  static int can_nocow_file_extent(struct btrfs_path *path,
 	args->disk_bytenr += args->start - key->offset;
 	args->num_bytes = min(args->end + 1, extent_end) - args->start;
 
+	args->file_extent.num_bytes = args->num_bytes;
+	args->file_extent.offset += args->start - key->offset;
+
 	/*
 	 * Force COW if csums exist in the range. This ensures that csums for a
 	 * given extent are either valid or do not exist.
@@ -7099,7 +7111,8 @@  static bool btrfs_extent_readonly(struct btrfs_fs_info *fs_info, u64 bytenr)
  */
 noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
 			      u64 *orig_start, u64 *orig_block_len,
-			      u64 *ram_bytes, bool nowait, bool strict)
+			      u64 *ram_bytes, struct btrfs_file_extent *file_extent,
+			      bool nowait, bool strict)
 {
 	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
 	struct can_nocow_file_extent_args nocow_args = { 0 };
@@ -7188,6 +7201,9 @@  noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
 		*orig_start = key.offset - nocow_args.extent_offset;
 	if (orig_block_len)
 		*orig_block_len = nocow_args.disk_num_bytes;
+	if (file_extent)
+		memcpy(file_extent, &nocow_args.file_extent,
+		       sizeof(struct btrfs_file_extent));
 
 	*len = nocow_args.num_bytes;
 	ret = 1;
@@ -7407,7 +7423,7 @@  static int btrfs_get_blocks_direct_write(struct extent_map **map,
 		block_start = em->block_start + (start - em->start);
 
 		if (can_nocow_extent(inode, start, &len, &orig_start,
-				     &orig_block_len, &ram_bytes, false, false) == 1) {
+				     &orig_block_len, &ram_bytes, NULL, false, false) == 1) {
 			bg = btrfs_inc_nocow_writers(fs_info, block_start);
 			if (bg)
 				can_nocow = true;
@@ -10660,7 +10676,7 @@  static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
 		free_extent_map(em);
 		em = NULL;
 
-		ret = can_nocow_extent(inode, start, &len, NULL, NULL, NULL, false, true);
+		ret = can_nocow_extent(inode, start, &len, NULL, NULL, NULL, NULL, false, true);
 		if (ret < 0) {
 			goto out;
 		} else if (ret) {