All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Naohiro Aota <naohiro.aota@wdc.com>
Cc: linux-btrfs@vger.kernel.org, linux-block@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>, David Sterba <dsterba@suse.com>,
	Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Subject: Re: [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct()
Date: Thu, 8 Jul 2021 15:54:38 +0200	[thread overview]
Message-ID: <20210708135438.GA2610@twin.jikos.cz> (raw)
In-Reply-To: <20210708131057.259327-4-naohiro.aota@wdc.com>

On Thu, Jul 08, 2021 at 10:10:57PM +0900, Naohiro Aota wrote:
> When on SINGLE block group, btrfs_get_io_geometry() will return "the
> size of the block group - the offset of the logical address within the
> block group" as geom.len. Since we allow up to 8 GB zone size on zoned
> btrfs, we can have up to 8 GB block group, so can have up to 8 GB
> geom.len. With this setup, we easily hit the "ASSERT(geom.len <=
> INT_MAX);".
> 
> The ASSERT looks like to guard btrfs_bio_clone_partial() and bio_trim()
> which both take "int" (now "unsigned int" with the previous patch). So to
> be precise the ASSERT should check if clone_len <= UINT_MAX. But
> actually, clone_len is already capped by bio.bi_iter.bi_size which is
> unsigned int. So the ASSERT is not necessary.
> 
> Drop the ASSERT and properly compare submit_len and geom.len in u64. Then,
> let the implicit casting to convert it to unsigned int.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/inode.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8f60314c36c5..b6cc26dd7919 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8206,8 +8206,8 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
>  	u64 start_sector;
>  	int async_submit = 0;
>  	u64 submit_len;
> -	int clone_offset = 0;
> -	int clone_len;
> +	unsigned int clone_offset = 0;
> +	unsigned int clone_len;
>  	u64 logical;
>  	int ret;
>  	blk_status_t status;
> @@ -8255,9 +8255,13 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
>  			status = errno_to_blk_status(ret);
>  			goto out_err_em;
>  		}
> -		ASSERT(geom.len <= INT_MAX);
>  
> -		clone_len = min_t(int, submit_len, geom.len);
> +		/*
> +		 * min()'s result is always capped by bio.bi_iter.bi_size
> +		 * which is unsigned int. So the implicit casting it to
> +		 * unsigned int is safe.
> +		 */
> +		clone_len = min(submit_len, geom.len);

I'd rather rely on explicit casts and asserts than a comment stating
what should happen. submit_len ang geom.len are both u64 and cast to u32
clone_len.

I think submit_len should be u32 too (to copy the bio.bi_size) to match
the types used for the purpose and also clone_len.

  reply	other threads:[~2021-07-08 13:57 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-08 13:10 [PATCH 0/3] fix argument type of bio_trim() Naohiro Aota
2021-07-08 13:10 ` [PATCH 1/3] block: fix arg " Naohiro Aota
2021-07-08 14:57   ` David Sterba
2021-07-09  0:42     ` Damien Le Moal
2021-07-09  4:53       ` Naohiro Aota
2021-07-09  4:39     ` Naohiro Aota
2021-07-09  4:55       ` Naohiro Aota
2021-07-08 13:10 ` [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() Naohiro Aota
2021-07-08 15:00   ` David Sterba
2021-07-09  5:01     ` Naohiro Aota
2021-07-08 13:10 ` [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() Naohiro Aota
2021-07-08 13:54   ` David Sterba [this message]
2021-07-08 15:03   ` David Sterba

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210708135438.GA2610@twin.jikos.cz \
    --to=dsterba@suse.cz \
    --cc=axboe@kernel.dk \
    --cc=chaitanya.kulkarni@wdc.com \
    --cc=dsterba@suse.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=naohiro.aota@wdc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.