The 02/03/2021 11:18, Mike Snitzer wrote:
> On Wed, Feb 03 2021 at 10:53am -0500,
> Sergei Shtepa <sergei.sht...@veeam.com> wrote:
> 
> > blk_interposer allows to intercept bio requests, remap bio to another 
> > devices or add new bios.
> > 
> > Signed-off-by: Sergei Shtepa <sergei.sht...@veeam.com>
> > ---
> >  block/bio.c               |  2 +
> >  block/blk-core.c          | 33 ++++++++++++++++
> >  block/genhd.c             | 82 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/blk_types.h |  6 ++-
> >  include/linux/genhd.h     | 18 +++++++++
> >  5 files changed, 139 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/bio.c b/block/bio.c
> > index 1f2cc1fbe283..f6f135eb84b5 100644
> > --- a/block/bio.c
> > +++ b/block/bio.c
> > @@ -684,6 +684,8 @@ void __bio_clone_fast(struct bio *bio, struct bio 
> > *bio_src)
> >     bio_set_flag(bio, BIO_CLONED);
> >     if (bio_flagged(bio_src, BIO_THROTTLED))
> >             bio_set_flag(bio, BIO_THROTTLED);
> > +   if (bio_flagged(bio_src, BIO_INTERPOSED))
> > +           bio_set_flag(bio, BIO_INTERPOSED);
> >     bio->bi_opf = bio_src->bi_opf;
> >     bio->bi_ioprio = bio_src->bi_ioprio;
> >     bio->bi_write_hint = bio_src->bi_write_hint;
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 7663a9b94b80..c84bc42ba88b 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1032,6 +1032,32 @@ static blk_qc_t __submit_bio_noacct_mq(struct bio 
> > *bio)
> >     return ret;
> >  }
> >  
> > +static blk_qc_t __submit_bio_interposed(struct bio *bio)
> > +{
> > +   struct bio_list bio_list[2] = { };
> > +   blk_qc_t ret = BLK_QC_T_NONE;
> > +
> > +   current->bio_list = bio_list;
> > +   if (likely(bio_queue_enter(bio) == 0)) {
> > +           struct gendisk *disk = bio->bi_disk;
> > +
> > +           if (likely(blk_has_interposer(disk))) {
> > +                   bio_set_flag(bio, BIO_INTERPOSED);
> > +                   disk->interposer->ip_submit_bio(bio);
> > +           } else /* interposer was removed */
> > +                   bio_list_add(&current->bio_list[0], bio);
> 
> style nit:
> 
> } else {
>       /* interposer was removed */
>       bio_list_add(&current->bio_list[0], bio);
> }
> 
> > +
> > +           blk_queue_exit(disk->queue);
> > +   }
> > +   current->bio_list = NULL;
> > +
> > +   /* Resubmit remaining bios */
> > +   while ((bio = bio_list_pop(&bio_list[0])))
> > +           ret = submit_bio_noacct(bio);
> > +
> > +   return ret;
> > +}
> > +
> >  /**
> >   * submit_bio_noacct - re-submit a bio to the block device layer for I/O
> >   * @bio:  The bio describing the location in memory and on the device.
> > @@ -1057,6 +1083,13 @@ blk_qc_t submit_bio_noacct(struct bio *bio)
> >             return BLK_QC_T_NONE;
> >     }
> >  
> > +   /*
> > +    * Checking the BIO_INTERPOSED flag is necessary so that the bio
> > +    * created by the blk_interposer do not get to it for processing.
> > +    */
> > +   if (blk_has_interposer(bio->bi_disk) &&
> > +       !bio_flagged(bio, BIO_INTERPOSED))
> > +           return __submit_bio_interposed(bio);
> >     if (!bio->bi_disk->fops->submit_bio)
> >             return __submit_bio_noacct_mq(bio);
> >     return __submit_bio_noacct(bio);
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 419548e92d82..39785a3ef703 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -30,6 +30,7 @@
> >  static struct kobject *block_depr;
> >  
> >  DECLARE_RWSEM(bdev_lookup_sem);
> > +DEFINE_MUTEX(bdev_interposer_mutex);
> 
> Seems you're using this mutex to protect access to disk->interposer in
> attach/detach.  This is to prevent attach/detach races to same device?

Yes. There is a probability of 0.00...01% that two different modules will
try to attach/detach to the same disk at the same time.
Since the attach/detach operation is infrequent, using mutex is quite 
appropriate.
> 
> Thankfully attach/detach isn't in the bio submission fast path but it'd
> be helpful to document what this mutex is protecting).

I'll think about the name of this mutex and add a comment.

> 
> A storm of attach or detach will all hit this global mutex though...
> 
> Mike
> 

Thank you for the review.
I am very interested in your opinion about [PATCH v4 4/6] and [PATCH v4 5/6].
However, the kernel test robot has already found something there on sparc.

-- 
Sergei Shtepa
Veeam Software developer.

Reply via email to