| Message ID | 20210520062255.4908-1-chaitanya.kulkarni@wdc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | block: fix bio_add_XXX_page() return type | expand |
On 20/05/2021 08:23, Chaitanya Kulkarni wrote: > Hi, > > The helper functions bio_add_XXX_page() returns the length which is > unsigned int but the return type of those functions is defined > as int instead of unsigned int. > > This is an attempt to fix the return type of those functions > and few callers. There are many places where this fix is needed > in the callers, if this series makes it to the upstream I'll convert > those callers gradually. > > Any feedback is welcome. > > -ck > > Chaitanya Kulkarni (8): > block: fix return type of bio_add_hw_page() > block: fix return type of bio_add_pc_page() > block: fix return type of bio_add_zone_append_page > block: fix return type of bio_add_page() > lightnvm: fix variable type pblk-core > pscsi: fix variable type pscsi_map_sg > btrfs: fix variable type in btrfs_bio_add_page > block: fix variable type for zero pages > > block/bio.c | 20 +++++++++++--------- > block/blk-lib.c | 2 +- > block/blk.h | 7 ++++--- > drivers/lightnvm/pblk-core.c | 3 ++- > drivers/target/target_core_pscsi.c | 6 ++++-- > fs/btrfs/extent_io.c | 2 +- > include/linux/bio.h | 11 ++++++----- > 7 files changed, 29 insertions(+), 22 deletions(-) > I couldn't spot any errors, but I'm not sure it's worth the effort. If Jens decides to take it: Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Wed, May 19, 2021 at 11:22:47PM -0700, Chaitanya Kulkarni wrote: > The helper functions bio_add_XXX_page() returns the length which is > unsigned int but the return type of those functions is defined > as int instead of unsigned int. I've been thinking about this for a few weeks as part of the folio patches: https://lore.kernel.org/linux-fsdevel/20210505150628.111735-72-willy@infradead.org/ - len and off are measured in bytes - neither are permitted to be negative - for efficiency we only permit them to be up to 4GB I therefore believe the correct type for these parameters to be size_t, and we should range-check them if they're too large. they should actually always fit within the page that they're associated with, but people do allocate non-compound pages and i'm not trying to break that today. using size_t makes it clear that these are byte counts, not (eg) sector counts. i do think it's good to make the return value unsigned so we don't have people expecting a negative errno on failure.
On 5/21/21 03:25, Johannes Thumshirn wrote: > I couldn't spot any errors, but I'm not sure it's worth the effort. > > If Jens decides to take it: > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > It does create confusion on the code level which can result in invalid error checks.
On 5/21/21 04:32, Matthew Wilcox wrote: > On Wed, May 19, 2021 at 11:22:47PM -0700, Chaitanya Kulkarni wrote: >> The helper functions bio_add_XXX_page() returns the length which is >> unsigned int but the return type of those functions is defined >> as int instead of unsigned int. > I've been thinking about this for a few weeks as part of the folio > patches: > > https://lore.kernel.org/linux-fsdevel/20210505150628.111735-72-willy@infradead.org/ > > - len and off are measured in bytes > - neither are permitted to be negative > - for efficiency we only permit them to be up to 4GB > > I therefore believe the correct type for these parameters to be size_t, > and we should range-check them if they're too large. they should > actually always fit within the page that they're associated with, but > people do allocate non-compound pages and i'm not trying to break that > today. > > using size_t makes it clear that these are byte counts, not (eg) sector > counts. i do think it's good to make the return value unsigned so we > don't have people expecting a negative errno on failure. > Sounds good, I'll wait for few days for the feedback from others and then send out the V1 with your suggestions for using size_t and bounds check.
On Fri, May 21, 2021 at 09:37:43PM +0000, Chaitanya Kulkarni wrote: > On 5/21/21 03:25, Johannes Thumshirn wrote: > > I couldn't spot any errors, but I'm not sure it's worth the effort. > > > > If Jens decides to take it: > > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > > > It does create confusion on the code level which can result in > invalid error checks. Do you have any examples of bugs caused by this confusion (whether they were fixed in the past, currently exist, or were caught in code review)? That would be good justification for doing this.
On 5/21/21 15:37, Omar Sandoval wrote: > On Fri, May 21, 2021 at 09:37:43PM +0000, Chaitanya Kulkarni wrote: >> On 5/21/21 03:25, Johannes Thumshirn wrote: >>> I couldn't spot any errors, but I'm not sure it's worth the effort. >>> >>> If Jens decides to take it: >>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>> >> It does create confusion on the code level which can result in >> invalid error checks. > Do you have any examples of bugs caused by this confusion (whether they > were fixed in the past, currently exist, or were caught in code review)? > That would be good justification for doing this. > Yes, while implementing the ZBD over NVMeOF I accidentally ended up checking the error value < 0 based on the function following declaration :- 1 F f bio_add_zone_append_page block/bio.c int bio_add_zone_append_page(struct bio *bio, struct page *page, I did fix it later when doing the next code review myself, maybe that is just me :P. Also, new functions inherit the same style such as bio_add_zone_append_page() from bio_add_page() and bio_add_pc_page() we can prevent that. What you do you think about the approach suggested by Matthew size_t ? If it is too much trouble we can drop it.
On Fri, May 21, 2021 at 12:30:45PM +0100, Matthew Wilcox wrote: > On Wed, May 19, 2021 at 11:22:47PM -0700, Chaitanya Kulkarni wrote: > > The helper functions bio_add_XXX_page() returns the length which is > > unsigned int but the return type of those functions is defined > > as int instead of unsigned int. > > I've been thinking about this for a few weeks as part of the folio > patches: > > https://lore.kernel.org/linux-fsdevel/20210505150628.111735-72-willy@infradead.org/ > > - len and off are measured in bytes > - neither are permitted to be negative > - for efficiency we only permit them to be up to 4GB > > I therefore believe the correct type for these parameters to be size_t, > and we should range-check them if they're too large. they should > actually always fit within the page that they're associated with, but > people do allocate non-compound pages and i'm not trying to break that > today. > > using size_t makes it clear that these are byte counts, not (eg) sector > counts. i do think it's good to make the return value unsigned so we > don't have people expecting a negative errno on failure. I think the right type is bool. We always return either 0 or the full length we tried to add. Instead of optimizing for a partial add (which only makes sense for bio_add_hw_page anyway), I'd rather make the interface as simple as possible.
On Mon, May 24, 2021 at 09:35:27AM +0200, Christoph Hellwig wrote: > > using size_t makes it clear that these are byte counts, not (eg) sector > > counts. i do think it's good to make the return value unsigned so we > > don't have people expecting a negative errno on failure. > > I think the right type is bool. We always return either 0 or the full > length we tried to add. Instead of optimizing for a partial add (which > only makes sense for bio_add_hw_page anyway), I'd rather make the > interface as simple as possible. Sounds good to me.
Christoph, On 5/24/21 00:35, Christoph Hellwig wrote: > On Fri, May 21, 2021 at 12:30:45PM +0100, Matthew Wilcox wrote: >> On Wed, May 19, 2021 at 11:22:47PM -0700, Chaitanya Kulkarni wrote: >>> The helper functions bio_add_XXX_page() returns the length which is >>> unsigned int but the return type of those functions is defined >>> as int instead of unsigned int. >> I've been thinking about this for a few weeks as part of the folio >> patches: >> >> https://lore.kernel.org/linux-fsdevel/20210505150628.111735-72-willy@infradead.org/ >> >> - len and off are measured in bytes >> - neither are permitted to be negative >> - for efficiency we only permit them to be up to 4GB >> >> I therefore believe the correct type for these parameters to be size_t, >> and we should range-check them if they're too large. they should >> actually always fit within the page that they're associated with, but >> people do allocate non-compound pages and i'm not trying to break that >> today. >> >> using size_t makes it clear that these are byte counts, not (eg) sector >> counts. i do think it's good to make the return value unsigned so we >> don't have people expecting a negative errno on failure. > I think the right type is bool. We always return either 0 or the full > length we tried to add. Instead of optimizing for a partial add (which > only makes sense for bio_add_hw_page anyway), I'd rather make the > interface as simple as possible. > Is above comment is on this series or on the API present in the folio patches [1] ? Since if we change the return type to bool for the functions in question [2] in this series we also need to modify the callers, I'm not sure that is worth it though. Please confirm. [1] https://lore.kernel.org/linux-fsdevel/20210505150628.111735-72-willy@infradead.org/ [2] bio_add_hw_page() bio_add_pc_page() bio_add_zone_append_page() bio_add page()
On Wed, May 26, 2021 at 02:55:57AM +0000, Chaitanya Kulkarni wrote: > Is above comment is on this series or on the API present in the folio > patches [1] ? All of the above. > Since if we change the return type to bool for the functions in > question [2] in this series we also need to modify the callers, I'm not sure > that is worth it though. Yes, all the caller would need to change as well.
On 5/27/21 04:43, Christoph Hellwig wrote: > On Wed, May 26, 2021 at 02:55:57AM +0000, Chaitanya Kulkarni wrote: >> Is above comment is on this series or on the API present in the folio >> patches [1] ? > All of the above. > >> Since if we change the return type to bool for the functions in >> question [2] in this series we also need to modify the callers, I'm not sure >> that is worth it though. > Yes, all the caller would need to change as well. > I see, thanks, I'll work on V1 with this approach.