| Message ID | 20210119050631.57073-1-chaitanya.kulkarni@wdc.com |
|---|---|
| Headers | show |
| Series | block: introduce bio_init_fields() | expand |
On Tue, Jan 19 2021 at 12:05am -0500, Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> 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 <snitzer@redhat.com>
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
On 1/18/21 21:06, 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. > 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, On Tue, Jan 19, 2021 at 5:01 PM Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> 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/
On 1/20/21 7:01 PM, Julian Calaby wrote: > Hi Chaitanya, > > On Tue, Jan 19, 2021 at 5:01 PM Chaitanya Kulkarni > <chaitanya.kulkarni@wdc.com> 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, 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.