| Message ID | 20240111160226.1936351-1-axboe@kernel.dk (mailing list archive) |
|---|---|
| Headers | show |
| Series | Integrity cleanups and optimization | expand |
On Thu, Jan 11, 2024 at 09:00:18AM -0700, Jens Axboe wrote: > Hi, > > 1 gets rid of the dummy nop profile, 2 marks the queue as having an > actual profile, and 3 avoids calling into bio_integrity_prep() if we > don't have a profile. This both reduces code (getting rid of the nop > profile) and reduces the overhead of the standard setup of having > integrity enabled in kconfig, yet not using a device with an integrity > profile. Bw, can someone help with what dm_integrity_profile is for? It is basically identical to the no-op one, just with a different name. With the no-op removal it is the only one outside of the pi once, and killing it would really help with some de-virtualization I've looked at a while ago.
> Bw, can someone help with what dm_integrity_profile is for? > It is basically identical to the no-op one, just with a different > name. With the no-op removal it is the only one outside of the pi > once, and killing it would really help with some de-virtualization > I've looked at a while ago. No particular objections from me wrt. using a flag. However, I believe the no-op profile and associated plumbing was a requirement for DM. I forget the details. Mike?
On Thu, Jan 11 2024 at 11:24P -0500, Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > Bw, can someone help with what dm_integrity_profile is for? > > It is basically identical to the no-op one, just with a different > > name. With the no-op removal it is the only one outside of the pi > > once, and killing it would really help with some de-virtualization > > I've looked at a while ago. > > No particular objections from me wrt. using a flag. > > However, I believe the no-op profile and associated plumbing was a > requirement for DM. I forget the details. Mike? I'll have to take a closer look.. staking device always complicates things. But the dummy functions that got wired up with this commit are suspect: 54d4e6ab91eb block: centralize PI remapping logic to the block layer Effectively the entirety of the dm_integrity_profile is "we don't do anything special".. so yes it would be nice to not require indirect calls to accomplish what dm-integrity needs from block core. Mike
On Thu, Jan 11 2024 at 11:34P -0500, Mike Snitzer <snitzer@kernel.org> wrote: > On Thu, Jan 11 2024 at 11:24P -0500, > Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > > > > Bw, can someone help with what dm_integrity_profile is for? > > > It is basically identical to the no-op one, just with a different > > > name. With the no-op removal it is the only one outside of the pi > > > once, and killing it would really help with some de-virtualization > > > I've looked at a while ago. > > > > No particular objections from me wrt. using a flag. > > > > However, I believe the no-op profile and associated plumbing was a > > requirement for DM. I forget the details. Mike? > > I'll have to take a closer look.. stacking device always complicates > things. The bulk of the following drivers/md/dm-table.c code snippets are from these 2 commits (first from me, 2nd from Martin): a63a5cf84dac dm: improve block integrity support 25520d55cdb6 block: Inline blk_integrity in struct gendisk static bool integrity_profile_exists(struct gendisk *disk) { return !!blk_get_integrity(disk); } /* * Get a disk whose integrity profile reflects the table's profile. * Returns NULL if integrity support was inconsistent or unavailable. */ static struct gendisk *dm_table_get_integrity_disk(struct dm_table *t) { struct list_head *devices = dm_table_get_devices(t); struct dm_dev_internal *dd = NULL; struct gendisk *prev_disk = NULL, *template_disk = NULL; for (unsigned int i = 0; i < t->num_targets; i++) { struct dm_target *ti = dm_table_get_target(t, i); if (!dm_target_passes_integrity(ti->type)) goto no_integrity; } list_for_each_entry(dd, devices, list) { template_disk = dd->dm_dev->bdev->bd_disk; if (!integrity_profile_exists(template_disk)) goto no_integrity; else if (prev_disk && blk_integrity_compare(prev_disk, template_disk) < 0) goto no_integrity; prev_disk = template_disk; } return template_disk; no_integrity: if (prev_disk) DMWARN("%s: integrity not set: %s and %s profile mismatch", dm_device_name(t->md), prev_disk->disk_name, template_disk->disk_name); return NULL; } /* * Register the mapped device for blk_integrity support if the * underlying devices have an integrity profile. But all devices may * not have matching profiles (checking all devices isn't reliable * during table load because this table may use other DM device(s) which * must be resumed before they will have an initialized integity * profile). Consequently, stacked DM devices force a 2 stage integrity * profile validation: First pass during table load, final pass during * resume. */ static int dm_table_register_integrity(struct dm_table *t) { struct mapped_device *md = t->md; struct gendisk *template_disk = NULL; /* If target handles integrity itself do not register it here. */ if (t->integrity_added) return 0; template_disk = dm_table_get_integrity_disk(t); if (!template_disk) return 0; if (!integrity_profile_exists(dm_disk(md))) { t->integrity_supported = true; /* * Register integrity profile during table load; we can do * this because the final profile must match during resume. */ blk_integrity_register(dm_disk(md), blk_get_integrity(template_disk)); return 0; } /* * If DM device already has an initialized integrity * profile the new profile should not conflict. */ if (blk_integrity_compare(dm_disk(md), template_disk) < 0) { DMERR("%s: conflict with existing integrity profile: %s profile mismatch", dm_device_name(t->md), template_disk->disk_name); return 1; } /* Preserve existing integrity profile */ t->integrity_supported = true; return 0; } /* * Verify that all devices have an integrity profile that matches the * DM device's registered integrity profile. If the profiles don't * match then unregister the DM device's integrity profile. */ static void dm_table_verify_integrity(struct dm_table *t) { struct gendisk *template_disk = NULL; if (t->integrity_added) return; if (t->integrity_supported) { /* * Verify that the original integrity profile * matches all the devices in this table. */ template_disk = dm_table_get_integrity_disk(t); if (template_disk && blk_integrity_compare(dm_disk(t->md), template_disk) >= 0) return; } if (integrity_profile_exists(dm_disk(t->md))) { DMWARN("%s: unable to establish an integrity profile", dm_device_name(t->md)); blk_integrity_unregister(dm_disk(t->md)); } } > But the dummy functions that got wired up with this commit are suspect: > 54d4e6ab91eb block: centralize PI remapping logic to the block layer > > Effectively the entirety of the dm_integrity_profile is "we don't do > anything special".. so yes it would be nice to not require indirect > calls to accomplish what dm-integrity needs from block core. All said, if blk_get_integrity() can be made to handle the removal of the nop profile then all should "just work" right? Here is how it has been: static inline struct blk_integrity *blk_get_integrity(struct gendisk *disk) { struct blk_integrity *bi = &disk->queue->integrity; if (!bi->profile) return NULL; return bi; } Not looked closely at Jens' patchset (but will do), however it seems Jens missed updating blk_get_integrity() to deal with his new QUEUE_FLAG_INTG_PROFILE flag? As for complete removal of integrity profiles entirely, DM needs a way to analyze integrity configurations to try to stack up a sane end result. Mike