Hi,
This is a *compile only RFC* which adds a generic helper to initialize
the various fields of the bio that is repeated all the places in
file-systems, block layer, and drivers.
The new helper allows callers to initialize various members such as
bdev, sector, private, end io callback, io priority, and write hints.
The objective of this RFC is to only start a discussion, this it not
completely tested at all. Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
Following diff shows code level benefits of this helper :-
 38 files changed, 124 insertions(+), 236 deletions(-)
-ck
Chaitanya Kulkarni (37):
block: introduce bio_init_fields() helper
fs: use bio_init_fields in block_dev
btrfs: use bio_init_fields in disk-io
btrfs: use bio_init_fields in volumes
ext4: use bio_init_fields in page_io
gfs2: use bio_init_fields in lops
gfs2: use bio_init_fields in meta_io
gfs2: use bio_init_fields in ops_fstype
iomap: use bio_init_fields in buffered-io
iomap: use bio_init_fields in direct-io
jfs: use bio_init_fields in logmgr
zonefs: use bio_init_fields in append
drdb: use bio_init_fields in actlog
drdb: use bio_init_fields in bitmap
drdb: use bio_init_fields in receiver
floppy: use bio_init_fields
pktcdvd: use bio_init_fields
bcache: use bio_init_fields in journal
bcache: use bio_init_fields in super
bcache: use bio_init_fields in writeback
dm-bufio: use bio_init_fields
dm-crypt: use bio_init_fields
dm-zoned: use bio_init_fields metadata
dm-zoned: use bio_init_fields target
dm-zoned: use bio_init_fields
dm log writes: use bio_init_fields
nvmet: use bio_init_fields in bdev-ns
target: use bio_init_fields in iblock
btrfs: use bio_init_fields in scrub
fs: use bio_init_fields in buffer
eros: use bio_init_fields in data
eros: use bio_init_fields in zdata
jfs: use bio_init_fields in metadata
nfs: use bio_init_fields in blocklayout
ocfs: use bio_init_fields in heartbeat
xfs: use bio_init_fields in xfs_buf
xfs: use bio_init_fields in xfs_log
block/blk-lib.c | 13 +++++--------
drivers/block/drbd/drbd_actlog.c | 5 +----
drivers/block/drbd/drbd_bitmap.c | 5 +----
drivers/block/drbd/drbd_receiver.c | 11 +++--------
drivers/block/floppy.c | 5 +----
drivers/block/pktcdvd.c | 12 ++++--------
drivers/md/bcache/journal.c | 21 ++++++++-------------
drivers/md/bcache/super.c | 19 +++++--------------
drivers/md/bcache/writeback.c | 14 ++++++--------
drivers/md/dm-bufio.c | 5 +----
drivers/md/dm-crypt.c | 4 +---
drivers/md/dm-log-writes.c | 21 ++++++---------------
drivers/md/dm-zoned-metadata.c | 15 +++++----------
drivers/md/dm-zoned-target.c | 9 +++------
drivers/md/md.c | 6 ++----
drivers/nvme/target/io-cmd-bdev.c | 4 +---
drivers/target/target_core_iblock.c | 11 +++--------
fs/block_dev.c | 17 +++++------------
fs/btrfs/disk-io.c | 11 ++++-------
fs/btrfs/scrub.c | 6 ++----
fs/btrfs/volumes.c | 4 +---
fs/buffer.c | 7 ++-----
fs/erofs/data.c | 6 ++----
fs/erofs/zdata.c | 9 +++------
fs/ext4/page-io.c | 6 ++----
fs/gfs2/lops.c | 6 ++----
fs/gfs2/meta_io.c | 5 ++---
fs/gfs2/ops_fstype.c | 7 ++-----
fs/iomap/buffered-io.c | 5 ++---
fs/iomap/direct-io.c | 15 +++++----------
fs/jfs/jfs_logmgr.c | 16 ++++------------
fs/jfs/jfs_metapage.c | 16 +++++++---------
fs/nfs/blocklayout/blocklayout.c | 8 ++------
fs/ocfs2/cluster/heartbeat.c | 4 +---
fs/xfs/xfs_buf.c | 6 ++----
fs/xfs/xfs_log.c | 6 ++----
fs/zonefs/super.c | 7 +++----
include/linux/bio.h | 13 +++++++++++++
38 files changed, 124 insertions(+), 236 deletions(-)
--
2.22.1
@@ -1693,12 +1691,9 @@ int drbd_submit_peer_request(struct drbd_device *device,gotofail;}/* > peer_req->i.sector, unless this is the first bio */-bio->bi_iter.bi_sector=sector;-bio_set_dev(bio,device->ldev->backing_bdev);+bio_init_fields(bio,device->ldev->backing_bdev,sector,peer_req,+drbd_peer_request_endio,0,0);bio_set_op_attrs(bio,op,op_flags);-bio->bi_private=peer_req;-bio->bi_end_io=drbd_peer_request_endio;-bio->bi_next=bios;bios=bio;++n_bios;
@@ -417,10 +417,10 @@ static int metapage_writepage(struct page *page, struct writeback_control *wbc)len=min(xlen,(int)JFS_SBI(inode->i_sb)->nbperpage);bio=bio_alloc(GFP_NOFS,1);-bio_set_dev(bio,inode->i_sb->s_bdev);-bio->bi_iter.bi_sector=pblock<<(inode->i_blkbits-9);-bio->bi_end_io=metapage_write_end_io;-bio->bi_private=page;+bio_init_fields(bio,inode->i_sb->s_bdev,+pblock<<(inode->i_blkbits-9),+page,metapage_write_end_io,0,0);+bio_set_op_attrs(bio,REQ_OP_WRITE,0);/* Don't call bio_add_page yet, we may add to this vec */
Ensuring that the device, sector and op are always initialized would
really helper some of the bio mapping helpers, so I'd rather
add a new
struct bio *bio_new(struct block_device *bdev, sector_t sector,
unsigned int op, unsigned int max_bvecs, gfp_t gfp_mask)
helper, where max_bvecs is clamped to BIO_MAX_PAGES.
bi_private, bi_end_io, bi_ioprio and bi_write_hint on the other hand
are purely optional and can be easily set just by the users that care.
@@ -528,10 +528,8 @@ static struct bio *o2hb_setup_one_bio(struct o2hb_region *reg,}/* Must put everything in 512 byte sectors for the bio... */+bio_init_fields(bio,reg->hr_bdev,0,wc,o2hb_bio_end_io,0,0);bio->bi_iter.bi_sector=(reg->hr_start_block+cs)<<(bits-9);-bio_set_dev(bio,reg->hr_bdev);-bio->bi_private=wc;-bio->bi_end_io=o2hb_bio_end_io;bio_set_op_attrs(bio,op,op_flags);vec_start=(cs<<bits)%PAGE_SIZE;
@@ -1006,13 +1006,10 @@ static void bm_page_io_async(struct drbd_bm_aio_ctx *ctx, int page_nr) __must_hobm_store_page_idx(page,page_nr);}elsepage=b->bm_pages[page_nr];-bio_set_dev(bio,device->ldev->md_bdev);-bio->bi_iter.bi_sector=on_disk_sector;+bio_init_fields(bio,device->ldev->md_bdev,on_disk_sector,ctx,drbd_bm_endio,0,0);/* bio_add_page of a single page to an empty bio will always succeed,*accordingtoapi.Dowewanttoassertthat?*/bio_add_page(bio,page,len,0);-bio->bi_private=ctx;-bio->bi_end_io=drbd_bm_endio;bio_set_op_attrs(bio,op,0);if(drbd_insert_fault(device,(op==REQ_OP_WRITE)?DRBD_FAULT_MD_WR:DRBD_FAULT_MD_RD)){
@@ -1980,15 +1980,11 @@ static int lbmRead(struct jfs_log * log, int pn, struct lbuf ** bpp)bp->l_flag|=lbmREAD;bio=bio_alloc(GFP_NOFS,1);--bio->bi_iter.bi_sector=bp->l_blkno<<(log->l2bsize-9);-bio_set_dev(bio,log->bdev);-+bio_init_fields(bio,log->bdev,bp->l_blkno<<(log->l2bsize-9),+log->bdev,0,lbmIODone,bp,0,0);bio_add_page(bio,bp->l_page,LOGPSIZE,bp->l_offset);BUG_ON(bio->bi_iter.bi_size!=LOGPSIZE);-bio->bi_end_io=lbmIODone;-bio->bi_private=bp;bio->bi_opf=REQ_OP_READ;/*check if journaling to disk has been disabled*/if(log->no_integrity){
@@ -2125,14 +2121,10 @@ static void lbmStartIO(struct lbuf * bp)jfs_info("lbmStartIO");bio=bio_alloc(GFP_NOFS,1);-bio->bi_iter.bi_sector=bp->l_blkno<<(log->l2bsize-9);-bio_set_dev(bio,log->bdev);-+bio_init_fields(bio,log->bdev,bp->l_blkno<<(log->l2bsize-9),+log->bdev,0,lbmIODone,bp,0,0);bio_add_page(bio,bp->l_page,LOGPSIZE,bp->l_offset);BUG_ON(bio->bi_iter.bi_size!=LOGPSIZE);--bio->bi_end_io=lbmIODone;-bio->bi_private=bp;bio->bi_opf=REQ_OP_WRITE|REQ_SYNC;/* check if journaling to disk has been disabled */
There are several places in the file-system, block layer, device drivers
where struct bio members such as bdev, sector, private, end io callback,
io priority, write hints are initialized where we can use a helper
function.
This pach introduces a helper function which we use in the block lyaer
code. Subsequent patches use this function to reduce repeated code.
Signed-off-by: Chaitanya Kulkarni <redacted>
---
block/blk-lib.c | 13 +++++--------
include/linux/bio.h | 13 +++++++++++++
2 files changed, 18 insertions(+), 8 deletions(-)
From: Josef Bacik <josef@toxicpanda.com> Date: 2021-01-19 21:42:47
On 1/19/21 12:05 AM, Chaitanya Kulkarni wrote:
Hi,
This is a *compile only RFC* which adds a generic helper to initialize
the various fields of the bio that is repeated all the places in
file-systems, block layer, and drivers.
The new helper allows callers to initialize various members such as
bdev, sector, private, end io callback, io priority, and write hints.
The objective of this RFC is to only start a discussion, this it not
completely tested at all.
It would help to know what you're trying to accomplish here. I'd echo Mike's
comments about how it makes it annoying to update things in the future. In
addition, there's so many fields that I'm not going to remember what each one is
without having to look it up, which makes it annoying to use and to review. If
it's simply to make sure fields are initialized then you could add debug sanity
checks to submit_bio(). If it's to clean up duplication, well I'd argue that
the duplication is much clearer than positional arguments in a giant function
call. If you are wanting to change a particular part of the bio to be
initialized properly, like Dennis's work to make sure the bi_blkg was
initialized at bi_bdev set time, then a more targeted patch series with a
specific intent will be more useful and more successful. Thanks,
Josef
From: Mike Snitzer <hidden> Date: 2021-01-19 23:01:13
On Tue, Jan 19 2021 at 12:05am -0500,
Chaitanya Kulkarni [off-list ref] wrote:
Hi,
This is a *compile only RFC* which adds a generic helper to initialize
the various fields of the bio that is repeated all the places in
file-systems, block layer, and drivers.
The new helper allows callers to initialize various members such as
bdev, sector, private, end io callback, io priority, and write hints.
The objective of this RFC is to only start a discussion, this it not
completely tested at all. Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
Following diff shows code level benefits of this helper :-
 38 files changed, 124 insertions(+), 236 deletions(-)
Please no... this is just obfuscation.
Adding yet another field to set would create a cascade of churn
throughout kernel (and invariably many callers won't need the new field
initialized, so you keep passing 0 for more and more fields).
Nacked-by: Mike Snitzer [off-list ref]
Hi,
This is a *compile only RFC* which adds a generic helper to initialize
the various fields of the bio that is repeated all the places in
file-systems, block layer, and drivers.
The new helper allows callers to initialize various members such as
bdev, sector, private, end io callback, io priority, and write hints.
The objective of this RFC is to only start a discussion, this it not
completely tested at all.
Following diff shows code level benefits of this helper :-
38 files changed, 124 insertions(+), 236 deletions(-)
-ck
Thanks for replying Mike, Josef and Christoph.
I'll move forward with Christoph's suggestion and get rid of
optional parameters which is making this API hard to use.
Hi Chaitanya,
(drop in-person Cc..)
On Mon, Jan 18, 2021 at 09:06:25PM -0800, Chaitanya Kulkarni wrote:
...it would be nice if you could update the subject line to
"erofs: use bio_init_fields xxxx"
The same to the following patch [RFC PATCH 32/37]... Also, IMHO,
these two patches could be merged as one patch if possible,
although just my own thoughts.
Thanks,
Gao Xiang
Hi Chaitanya,
(drop in-person Cc..)
On Mon, Jan 18, 2021 at 09:06:25PM -0800, Chaitanya Kulkarni wrote:
...it would be nice if you could update the subject line to
"erofs: use bio_init_fields xxxx"
Sure, if new helper is accepted and can be used in erosfs
then I'll make that change. Thanks for pointing out.
The same to the following patch [RFC PATCH 32/37]... Also, IMHO,
these two patches could be merged as one patch if possible,
although just my own thoughts.
Thanks,
Gao Xiang
Hi Chaitanya,
On Tue, Jan 19, 2021 at 5:01 PM Chaitanya Kulkarni
[off-list ref] wrote:
Hi,
This is a *compile only RFC* which adds a generic helper to initialize
the various fields of the bio that is repeated all the places in
file-systems, block layer, and drivers.
The new helper allows callers to initialize various members such as
bdev, sector, private, end io callback, io priority, and write hints.
The objective of this RFC is to only start a discussion, this it not
completely tested at all.
Following diff shows code level benefits of this helper :-
38 files changed, 124 insertions(+), 236 deletions(-)
On a more abstract note, I don't think this diffstat is actually
illustrating the benefits of this as much as you think it is.
Yeah, we've reduced the code by 112 lines, but that's barely half the
curn here. It looks, from the diffstat, that you've effectively
reduced 2 lines into 1. That isn't much of a saving.
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Hi Chaitanya,
On Tue, Jan 19, 2021 at 5:01 PM Chaitanya Kulkarni
[off-list ref] wrote:
quoted
Hi,
This is a *compile only RFC* which adds a generic helper to initialize
the various fields of the bio that is repeated all the places in
file-systems, block layer, and drivers.
The new helper allows callers to initialize various members such as
bdev, sector, private, end io callback, io priority, and write hints.
The objective of this RFC is to only start a discussion, this it not
completely tested at all.
Following diff shows code level benefits of this helper :-
38 files changed, 124 insertions(+), 236 deletions(-)
On a more abstract note, I don't think this diffstat is actually
illustrating the benefits of this as much as you think it is.
Yeah, we've reduced the code by 112 lines, but that's barely half the
curn here. It looks, from the diffstat, that you've effectively
reduced 2 lines into 1. That isn't much of a saving.
Thanks,
The diff stat is not the only measure since every component fs/driver
has a different style and nested call it just to show the effect.
Thanks for your comment, we have decided to go with the bio_new approach.