Re: [PATCH v2 1/3] block: fix arg type of bio_trim()

[Naohiro Aota <Naohiro.Aota@wdc.com>]

-Chaitanya, as the address is unavailable.

On Wed, Jul 21, 2021 at 07:36:05AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 21, 2021 at 03:26:58PM +0900, Naohiro Aota wrote:
> > +void bio_trim(struct bio *bio, sector_t offset, sector_t size)
> >  {
> > +	const sector_t uint_max_sectors = UINT_MAX >> SECTOR_SHIFT;
> 
> I'd make this a #define and keep it close to the struct bio definition
> named something like BIO_MAX_SECTORS

Sure. I'll do so.

> > +
> > +	/*
> > +	 * 'bio' is a cloned bio which we need to trim to match the given
> > +	 * offset and size.
> >  	 */
> 
> This should go into the kernel doc comment.
> 
> Something like:
> 
> /**
>  * bio_trim - trim a bio to the given offset and size
>  * @bio:        bio to trim
>  * @offset:     number of sectors to trim from the front of @bio
>  * @size:       size we want to trim @bio to, in sectors
>  *
>  * This function is typically used for bios that are cloned and
>  * submitted to the underlying device in parts.
>  */

Thanks. I'll use this.
 
> > +	/* sanity check */
> > +	if (WARN_ON(offset > uint_max_sectors || size > uint_max_sectors ||
> > +		    offset + size > bio->bi_iter.bi_size))
> > +		return;
> 
> No real need for the comment. WARN_ON pretty much implies a sanity
> check.  I'd make this a WARN_ON_ONCE as otherwise you'll probably
> drown in these warnings if they ever hit.

Done.

> > -extern void bio_trim(struct bio *bio, int offset, int size);
> > +extern void bio_trim(struct bio *bio, sector_t offset, sector_t size);
> 
> Please drop the extern while you're at it.

Ah, I didn't know that. I'll keep it in mind.