Re: [PATCH V3 2/8] md: add io accounting for raid0 and raid5

[Guoqing Jiang <jgq516@gmail.com>]

On 5/27/21 11:25 PM, Christoph Hellwig wrote:

>   
>   	pr_debug("md: data integrity enabled on %s\n", mdname(mddev));
> -	if (bioset_integrity_create(&mddev->bio_set, BIO_POOL_SIZE)) {
> +	if (bioset_integrity_create(&mddev->bio_set, BIO_POOL_SIZE) ||
> +	    bioset_integrity_create(&mddev->io_acct_set, BIO_POOL_SIZE)) {
> Don't we need to create this new only for raid0 and raid5?
> Shouldn't they call helpers to create it?

Good catch, will add a check for level.

>> @@ -5864,6 +5866,12 @@ int md_run(struct mddev *mddev)
>>   		if (err)
>>   			return err;
>>   	}
>> +	if (!bioset_initialized(&mddev->io_acct_set)) {
>> +		err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
>> +				  offsetof(struct md_io_acct, bio_clone), 0);
>> +		if (err)
>> +			return err;
>> +	}
> Can someone explain why we are having these bioset_initialized checks
> here (also for the existing one)?  This just smells like very sloppy
> life time rules.

My understanding is that md_run is not only called when array is
created/assembled, for example, it can also be called in md_ioctl,
which means you can't call bioset_init unconditionally. Others may
have better explanation.

BTW, besides md, dm is another user of bioset_initialized.

>> +/* used by personalities (raid0 and raid5) to account io stats */
> Instead of mentioning the personalities this migt better explain
> something like ".. by personalities that don't already clone the
> bio and thus can't easily add the timestamp to their extended bio
> structure"

Ok, thanks for rephrasing.

>> +void md_account_bio(struct mddev *mddev, struct bio **bio)
>> +{
>> +	struct md_io_acct *md_io_acct;
>> +	struct bio *clone;
>> +
>> +	if (!blk_queue_io_stat((*bio)->bi_bdev->bd_disk->queue))
>> +		return;
>> +
>> +	clone = bio_clone_fast(*bio, GFP_NOIO, &mddev->io_acct_set);
>> +	md_io_acct = container_of(clone, struct md_io_acct, bio_clone);
>> +	md_io_acct->orig_bio = *bio;
>> +	md_io_acct->start_time = bio_start_io_acct(*bio);
>> +
>> +	clone->bi_end_io = md_end_io_acct;
>> +	clone->bi_private = md_io_acct;
>> +	*bio = clone;
> I would find a calling conventions that returns the allocated clone
> (or the original bio if there is no accounting) more logical.

Not sure if I follow, do you want the function return "struct bio *"
instead of "void"? I don't think there is fundamental difference
with current behavior.

>> +	struct bio_set			io_acct_set; /* for raid0 and raid5 io accounting */
> crazy long line.

At lease it aligns with above line and checkpatch doesn't complain
either.

Thanks,
Guoqing