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@xxxxxxxxxxxxx/ >> >> - 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@xxxxxxxxxxxxx/ [2] bio_add_hw_page() bio_add_pc_page() bio_add_zone_append_page() bio_add page()
