-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.
