bio_add_folio argument order

bio_add_folio argument order

[Matthew Wilcox]

bio_add_page() has its arguments in the wrong order:

extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);

Oh, right, and the prototype commits the cardinal sin of just giving you
a pair of unsigned ints and doesn't bother to tell you what they mean.
I'll send a patch for that ... anyway:

int bio_add_page(struct bio *bio, struct page *page,
                 unsigned int len, unsigned int offset)

This fails to follow #4: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

Here's what I want to do for the folio equivalent:

size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t off,
                size_t len)

This will make the transition more painful, but it does remove an irritant
for the future.  Any objections?

Re: bio_add_folio argument order

[Jens Axboe]

On 4/28/21 10:50 AM, Matthew Wilcox wrote:
> bio_add_page() has its arguments in the wrong order:
> 
> extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
> 
> Oh, right, and the prototype commits the cardinal sin of just giving you
> a pair of unsigned ints and doesn't bother to tell you what they mean.
> I'll send a patch for that ... anyway:
> 
> int bio_add_page(struct bio *bio, struct page *page,
>                  unsigned int len, unsigned int offset)
> 
> This fails to follow #4: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
> 
> Here's what I want to do for the folio equivalent:
> 
> size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t off,
>                 size_t len)
> 
> This will make the transition more painful, but it does remove an irritant
> for the future.  Any objections?

What's the point in shuffling len and offset around?

-- 
Jens Axboe

Re: bio_add_folio argument order

[Matthew Wilcox]

On Wed, Apr 28, 2021 at 10:58:44AM -0600, Jens Axboe wrote:
> On 4/28/21 10:50 AM, Matthew Wilcox wrote:
> > bio_add_page() has its arguments in the wrong order:
> > 
> > extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
> > 
> > Oh, right, and the prototype commits the cardinal sin of just giving you
> > a pair of unsigned ints and doesn't bother to tell you what they mean.
> > I'll send a patch for that ... anyway:
> > 
> > int bio_add_page(struct bio *bio, struct page *page,
> >                  unsigned int len, unsigned int offset)
> > 
> > This fails to follow #4: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
> > 
> > Here's what I want to do for the folio equivalent:
> > 
> > size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t off,
> >                 size_t len)
> > 
> > This will make the transition more painful, but it does remove an irritant
> > for the future.  Any objections?
> 
> What's the point in shuffling len and offset around?

That almost everything else does (off, len).  Grepping include, here's
the (object, len, off) examples I found (will miss multi-line examples,
tried to use my best judgement, tried to exclude loff_t):

bio_add_zone_append_page
__bio_try_merge_page
__bio_add_page
bio_integrity_add_page
fscrypt_encrypt_block_inplace
fscrypt_decrypt_block_inplace
sg_set_page
sg_copy_buffer
sg_pcopy_from_buffer
sg_pcopy_to_buffer
sg_zero_buffer
copy_linear_skb

versus (object, off, len):

bpf_ctx_copy_t
->is_partially_uptodate
memcpy_from_page
memcpy_to_page
memzero_page
(basically all of the iomap functions)
kvm_write_guest_page
kvm_vcpu_write_guest_page
skb_gro_remcsum_process
nf_checksum_partial
perf_copy_f
read_module_eeprom
skb_frag_foreach_page
skb_to_sgvec_nomark
skb_to_sgvec
skb_send_sock
various skb_checksum_ops
sk_msg_clone
various gss_krb functions
xdr_process_buf
usercopy_warn
various network getfrag functions

Not _quite_ as one-sided as I thought, but there are basically three
families of functions (bio, fscrypt, sg) that are len-first (plus
copy_linear_skb() is out-of-family), then networking, filesystems,
highmem, perf, bpf, kvm are off-first.

So, you're the maintainer, it's your call ... do you want bio_add_folio()
to resemble the bio_add_page() calls as much as possible, or do you want
to migrate towards how the rest of the world works?

Re: bio_add_folio argument order

[Jens Axboe]

On 4/28/21 2:00 PM, Matthew Wilcox wrote:
> On Wed, Apr 28, 2021 at 10:58:44AM -0600, Jens Axboe wrote:
>> On 4/28/21 10:50 AM, Matthew Wilcox wrote:
>>> bio_add_page() has its arguments in the wrong order:
>>>
>>> extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
>>>
>>> Oh, right, and the prototype commits the cardinal sin of just giving you
>>> a pair of unsigned ints and doesn't bother to tell you what they mean.
>>> I'll send a patch for that ... anyway:
>>>
>>> int bio_add_page(struct bio *bio, struct page *page,
>>>                  unsigned int len, unsigned int offset)
>>>
>>> This fails to follow #4: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
>>>
>>> Here's what I want to do for the folio equivalent:
>>>
>>> size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t off,
>>>                 size_t len)
>>>
>>> This will make the transition more painful, but it does remove an irritant
>>> for the future.  Any objections?
>>
>> What's the point in shuffling len and offset around?
> 
> That almost everything else does (off, len).  Grepping include, here's
> the (object, len, off) examples I found (will miss multi-line examples,
> tried to use my best judgement, tried to exclude loff_t):
> 
> bio_add_zone_append_page
> __bio_try_merge_page
> __bio_add_page
> bio_integrity_add_page
> fscrypt_encrypt_block_inplace
> fscrypt_decrypt_block_inplace
> sg_set_page
> sg_copy_buffer
> sg_pcopy_from_buffer
> sg_pcopy_to_buffer
> sg_zero_buffer
> copy_linear_skb
> 
> versus (object, off, len):
> 
> bpf_ctx_copy_t
> ->is_partially_uptodate
> memcpy_from_page
> memcpy_to_page
> memzero_page
> (basically all of the iomap functions)
> kvm_write_guest_page
> kvm_vcpu_write_guest_page
> skb_gro_remcsum_process
> nf_checksum_partial
> perf_copy_f
> read_module_eeprom
> skb_frag_foreach_page
> skb_to_sgvec_nomark
> skb_to_sgvec
> skb_send_sock
> various skb_checksum_ops
> sk_msg_clone
> various gss_krb functions
> xdr_process_buf
> usercopy_warn
> various network getfrag functions
> 
> Not _quite_ as one-sided as I thought, but there are basically three
> families of functions (bio, fscrypt, sg) that are len-first (plus
> copy_linear_skb() is out-of-family), then networking, filesystems,
> highmem, perf, bpf, kvm are off-first.
> 
> So, you're the maintainer, it's your call ... do you want
> bio_add_folio() to resemble the bio_add_page() calls as much as
> possible, or do you want to migrate towards how the rest of the world
> works?

Given that, I'd rather stick with the current ordering. The risk of
mistakes is much smaller that way. And not sure 'rest of world' is
really that applicable here in the kernel, the list above seems only
slightly skewed to one side. Hence I'd rather keep it safe and just
retain it.

-- 
Jens Axboe