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
