On Wed, Dec 04 2019 at 11:17am -0500, Nikos Tsironis <ntsironis@xxxxxxxxxxx> wrote: > On 12/4/19 5:27 PM, Joe Thornber wrote: > >On Wed, Dec 04, 2019 at 04:07:42PM +0200, Nikos Tsironis wrote: > >>The thin provisioning target maintains per thin device mappings that map > >>virtual blocks to data blocks in the data device. > > > > > >Ack. But I think we're issuing the FLUSH twice with your patch. Since the > >original bio is still remapped and issued at the end of process_deferred_bios? > > > > Yes, that's correct. I thought of it and of putting a check in > process_deferred_bios() to complete FLUSH bios immediately, but I have > one concern and I preferred to be safe than sorry. > > In __commit_transaction() there is the following check: > > if (unlikely(!pmd->in_service)) > return 0; > > , which means we don't commit the metadata, and thus we don't flush the > data device, in case the pool is not in service. > > Opening a thin device doesn't seem to put the pool in service, since > dm_pool_open_thin_device() uses pmd_write_lock_in_core(). > > Can I assume that the pool is in service if I/O can be mapped to a thin > device? If so, it's safe to put such a check in process_deferred_bios(). In service means upper layer has issued a write to a thin device of a pool. The header for commit 873f258becca87 gets into more detail. > On second thought though, in order for a flush bio to end up in > deferred_flush_bios in the first place, someone must have changed the > metadata and thus put the pool in service. Otherwise, it would have been > submitted directly to the data device. So, it's probably safe to check > for flush bios after commit() in process_deferred_bios() and complete > them immediately. Yes, I think so, which was Joe's original point. > If you confirm too that this is safe, I will send a second version of > the patch adding the check. Not seeing why we need another in_service check. After your changes are applied, any commit will trigger a preceeding flush.. so the deferred flushes are redundant. By definition, these deferred bios imply the pool is in service. I'd be fine with seeing a 3rd follow-on thinp patch that completes the redundant flushes immediately. Thanks, Mike -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
