* bio_add_folio argument order
@ 2021-04-28 16:50 Matthew Wilcox
2021-04-28 16:58 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2021-04-28 16:50 UTC (permalink / raw)
To: linux-block; +Cc: Christoph Hellwig, Jens Axboe
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?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: bio_add_folio argument order 2021-04-28 16:50 bio_add_folio argument order Matthew Wilcox @ 2021-04-28 16:58 ` Jens Axboe 2021-04-28 20:00 ` Matthew Wilcox 0 siblings, 1 reply; 4+ messages in thread From: Jens Axboe @ 2021-04-28 16:58 UTC (permalink / raw) To: Matthew Wilcox, linux-block; +Cc: Christoph Hellwig 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: bio_add_folio argument order 2021-04-28 16:58 ` Jens Axboe @ 2021-04-28 20:00 ` Matthew Wilcox 2021-04-28 20:08 ` Jens Axboe 0 siblings, 1 reply; 4+ messages in thread From: Matthew Wilcox @ 2021-04-28 20:00 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, Christoph Hellwig 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? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: bio_add_folio argument order 2021-04-28 20:00 ` Matthew Wilcox @ 2021-04-28 20:08 ` Jens Axboe 0 siblings, 0 replies; 4+ messages in thread From: Jens Axboe @ 2021-04-28 20:08 UTC (permalink / raw) To: Matthew Wilcox; +Cc: linux-block, Christoph Hellwig 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-28 20:08 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-28 16:50 bio_add_folio argument order Matthew Wilcox 2021-04-28 16:58 ` Jens Axboe 2021-04-28 20:00 ` Matthew Wilcox 2021-04-28 20:08 ` Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.