Re: [PATCH v6 2/4] block: add blk_interposer

[Sergei Shtepa]

Thank you, Christoph, for the review.
I will correct all except two points.

The 03/09/2021 20:27, Christoph Hellwig wrote:
> > +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 block_device *bdev = bio->bi_bdev;
> > +
> > +		if (likely(bdev_has_interposer(bdev))) {
> > +			bio_set_flag(bio, BIO_INTERPOSED);
> > +			bdev->bd_interposer->ip_submit_bio(bio);
> > +		} else {
> > +			/* interposer was removed */
> > +			bio_list_add(&current->bio_list[0], bio);
> > +		}
> > +
> > +		blk_queue_exit(bdev->bd_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.
> > @@ -1043,6 +1071,14 @@ 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 bdev_interposer do not get to it for processing.
> > +	 */
> > +	if (bdev_has_interposer(bio->bi_bdev) &&
> > +	    !bio_flagged(bio, BIO_INTERPOSED))
> > +		return __submit_bio_interposed(bio);
> > +
> >  	if (!bio->bi_bdev->bd_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 fcc530164b5a..1ae8516643c8 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -30,6 +30,11 @@
> >  static struct kobject *block_depr;
> >  
> >  DECLARE_RWSEM(bdev_lookup_sem);
> > +/*
> > + * Prevents different block-layer interposers from attaching or detaching
> > + * to the block device at the same time.
> > + */
> > +DEFINE_MUTEX(bdev_interposer_attach_lock);
> 
> This one can and should be marked static.
> 
> > +int bdev_interposer_attach(struct block_device *bdev, struct bdev_interposer *interposer,
> 
> Please avoid the overly long line.
> 
> > +	int ret = 0;
> > +
> > +	if (WARN_ON(!interposer))
> 
> WARN_ON_ONCE?

This function should be called quite rarely, and the absence of the interposer
parameter indicates that the function is being used incorrectly.
I would like to see this warning every time.

> 
> > +		return -EINVAL;
> > +
> > +	if (!blk_mq_is_queue_frozen(bdev->bd_disk->queue))
> > +		return -EPERM;
> 
> This probly should be a WARN_ON_ONCE() as well.

I think it's better to apply WARN_ON here.

> 
> > +
> > +	mutex_lock(&bdev_interposer_attach_lock);
> > +	if (bdev_has_interposer(bdev)) {
> > +		if (bdev->bd_interposer->ip_submit_bio == ip_submit_bio)
> > +			ret = -EALREADY;
> > +		else
> > +			ret = -EBUSY;
> > +		goto out;
> > +	}
> 
> Do we really need the two different error codes here?

I think I need it. If we try to initialize the interposer again, the reason
for this error is most likely in the logic of the module itself.
If the interposer is occupied by someone else, then we need to let know
about it.

> 
> > +
> > +	interposer->ip_submit_bio = ip_submit_bio;
> 
> I'd rather let the caller initialize the field instead of passing the
> submit function separately.

Yes, I think so. This will allow to keep only one parameter of the function.

> 
> > +void bdev_interposer_detach(struct bdev_interposer *interposer,
> > +			  const ip_submit_bio_t ip_submit_bio)
> > +{
> 
> > +	/* Check if it is really our interposer. */
> > +	if (WARN_ON(bdev->bd_interposer->ip_submit_bio != ip_submit_bio))
> > +		goto out;
> 
> I don't really see any need to pass ip_submit_bio just for this check.
> 
> > +	struct bdev_interposer * bd_interposer;
> 
> The * goes just before the member name.
> 
> > +/*
> > + * block layer interposers structure and functions
> > + */
> > +typedef void (*ip_submit_bio_t) (struct bio *bio);
> > +
> > +struct bdev_interposer {
> > +	ip_submit_bio_t ip_submit_bio;
> > +	struct block_device *bdev;
> 
> Do we need the ip_ prefix here?  Also we probably don't really the
> the typedef for the function pointer.

Ok. Maybe submit_bio_hook would be better? or submit_bio_interposer.

> 
> > +#define bdev_has_interposer(bd) ((bd)->bd_interposer != NULL)
> 
> And inline function would be nice here.

-- 
Sergei Shtepa
Veeam Software developer.