Re: [PATCH 2/2] dm thin: Flush data device before committing metadata

[Mike Snitzer <snitzer@redhat.com>]

On Wed, Dec 04 2019 at 11:17am -0500,
Nikos Tsironis <ntsironis@arrikto.com> 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