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
