linux-kernel - Re: [PATCH 2/3] btrfs: cache RAID stripe tree decission in btrfs_io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL3q7H4XY7FBMHUUBjjStZCfwvR=ZWTGsZ-xnPdmagaF6HJ+bw@mail.gmail.com>
Date: Fri, 13 Dec 2024 12:01:31 +0000
From: Filipe Manana <fdmanana@...nel.org>
To: Johannes Thumshirn <jth@...nel.org>
Cc: Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, 
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Johannes Thumshirn <johannes.thjumshirn@....com>, 
	Johannes Thumshirn <johannes.thumshirn@....com>
Subject: Re: [PATCH 2/3] btrfs: cache RAID stripe tree decission in btrfs_io_context

On Thu, Dec 12, 2024 at 12:55 PM Johannes Thumshirn <jth@...nel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@....com>
>
> Cache the decission if a particular I/O needs to update RAID stripe tree

decission -> decision

The subject also has this typo.

> entries in struct btrfs_io_context.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@....com>
> ---
>  fs/btrfs/bio.c     | 3 +--
>  fs/btrfs/volumes.c | 1 +
>  fs/btrfs/volumes.h | 2 ++
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 7ea6f0b43b95072b380172dc16e3c0de208a952b..bc80ee4f95a5a8de05f2664f68ac4fcb62864d7b 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -725,8 +725,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>                         bio->bi_opf |= REQ_OP_ZONE_APPEND;
>                 }
>
> -               if (is_data_bbio(bbio) && bioc &&
> -                   btrfs_need_stripe_tree_update(bioc->fs_info, bioc->map_type)) {
> +               if (is_data_bbio(bbio) && bioc && bioc->use_rst) {
>                         /*
>                          * No locking for the list update, as we only add to
>                          * the list in the I/O submission path, and list
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index fa190f7108545eacf82ef2b5f1f3838d56ca683e..088ba0499e184c93a402a3f92167cccfa33eec58 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6663,6 +6663,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>                 goto out;
>         }
>         bioc->map_type = map->type;
> +       bioc->use_rst = io_geom.use_rst;
>
>         /*
>          * For RAID56 full map, we need to make sure the stripes[] follows the
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 3a416b1bc24cb0735c783de90fb7490d795d7d96..0a00ee36f66b6d6831c43abda4a791684c11ea02 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -490,6 +490,8 @@ struct btrfs_io_context {
>         u64 size;
>         /* Raid stripe tree ordered entry. */
>         struct list_head rst_ordered_entry;
> +       /* This I/O operation uses the RAID stripe tree */

The comment seems kind of pointless as the variable name makes it
clear what the purpose is.
In the previous patch there's no comment about the new field in the
btrfs_io_geometry structure, which is fine, so I don't see why it's
needed here.

Also, for style consistency we should finish the sentence with punctuation.

> +       bool use_rst;

This increases the structure size from 88 bytes to 96 bytes on a
release kernel for x86_64.

As we can have many of these structures allocated at any given time,
it would be better to avoid increasing the size.
One way is to place the field in a hole such as right after the
'max_errors' field, so that the size of the structure doesn't change.

Thanks.

>
>         /*
>          * The total number of stripes, including the extra duplicated
>
> --
> 2.43.0
>
>