Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio

From: David Sterba <dsterba@suse.cz>
To: Christoph Hellwig <hch@lst.de>
Cc: Nikolay Borisov <nborisov@suse.com>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 3/3] btrfs: pass the btrfs_bio_ctrl to submit_one_bio
Date: Mon, 6 Jun 2022 22:23:22 +0200	[thread overview]
Message-ID: <20220606202322.GE20633@twin.jikos.cz> (raw)
In-Reply-To: <20220606162929.GA10835@lst.de>

On Mon, Jun 06, 2022 at 06:29:29PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 06, 2022 at 01:41:50PM +0300, Nikolay Borisov wrote:
> >> +static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
> >> +{
> >> +	if (bio_ctrl->bio)
> >> +		__submit_one_bio(bio_ctrl);
> >>   }
> >
> > Why do you need a function just to put an if in it, just move this atop 
> > __submit_one_bio :
> >
> > if (!bio_ctrl->bio)
> >     return
> >
> > and rename it to submit_one_bio.
> 
> Because just moving it to the top will lead to null pointer dereferences.
> I'd also have to move some initialization down.  Based on that the
> wrapper seems cleaner and safer to me.

I don't see much reason to have the safe and unsafe variants, it's all
for internal use in one file, also there's only one real instance where
__submit_one_bio can be used.  I'd expect a normal and __ variant for
some public API. Moving the initialization does not seem to be making
the code hard to read, so I'd apply this diff on top of your patch:

--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -179,11 +179,18 @@ static int add_extent_changeset(struct extent_state *state, u32 bits,
        return ret;
 }
 
-static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
+static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
 {
-       struct bio *bio = bio_ctrl->bio;
-       struct inode *inode = bio_first_page_all(bio)->mapping->host;
-       int mirror_num = bio_ctrl->mirror_num;
+       struct bio *bio;
+       struct inode *inode;
+       int mirror_num;
+
+       if (!bio_ctrl->bio)
+               return;
+
+       bio = bio_ctrl->bio;
+       inode = bio_first_page_all(bio)->mapping->host;
+       mirror_num = bio_ctrl->mirror_num;
 
        /* Caller should ensure the bio has at least some range added */
        ASSERT(bio->bi_iter.bi_size);
@@ -200,12 +207,6 @@ static void __submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
        bio_ctrl->bio = NULL;
 }
 
-static void submit_one_bio(struct btrfs_bio_ctrl *bio_ctrl)
-{
-       if (bio_ctrl->bio)
-               __submit_one_bio(bio_ctrl);
-}
-
 /*
  * Submit or fail the current bio in an extent_page_data structure.
  */
@@ -223,7 +224,7 @@ static void submit_write_bio(struct extent_page_data *epd, int ret)
                /* The bio is owned by the bi_end_io handler now */
                epd->bio_ctrl.bio = NULL;
        } else {
-               __submit_one_bio(&epd->bio_ctrl);
+               submit_one_bio(&epd->bio_ctrl);
        }
 }
---
