Re: [PATCH v18 12/21] dm: add finalize hook to target_type

[Fan Wu]

On 5/9/2024 10:07 AM, Mikulas Patocka wrote:
> On Wed, 8 May 2024, Fan Wu wrote:
>
> > On 5/8/2024 10:17 AM, Mikulas Patocka wrote:
> > > On Fri, 3 May 2024, Fan Wu wrote:
> > >
> > > > This patch adds a target finalize hook.
> > > >
> > > > The hook is triggered just before activating an inactive table of a
> > > > mapped device. If it returns an error the __bind get cancelled.
> > > >
> > > > The dm-verity target will use this hook to attach the dm-verity's
> > > > roothash metadata to the block_device struct of the mapped device.
> > > >
> > > > Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
> > >
> > > Hi
> > >
> > > Why not use the preresume callback?
> > >
> > > Is there some reason why do we need a new callback instead of using the
> > > existing one?
> > >
> > > Mikulas
> > Thanks for the suggestion.
> >
> > Mike suggested the new finalize() callback. I think the reason for not using
> > the preresume() callback is that there are multiple points that can fail
> > before activating an inactive table of a mapped device which can potentially
> > lead to inconsistent state.
> >
> > In our specific case, we are trying to associate dm-verity's roothash metadata
> > with the block_device struct of the mapped device inside the callback.
> >
> > If we use the preresume() callback for the work and an error occurs between
> > the callback and the table activation, this leave the block_device struct in
> > an inconsistent state.
>
> The preresume callback is the final GO/NO-GO decision point. If all the
> targets return zero in their preresume callback, then there's no turning
> back and the table will be activated.
>
> > This is because now the block device contains the roothash metadata of it's
> > inactive table due to the preresume() callback, but the activation failed so
> > the mapped device is still using the old table.
> >
> > The new finalize() callback guarantees that the callback happens just before
> > the table activation, thus avoiding the inconsistency.
>
> In your patch, it doesn't guarantee that.
>
> do_resume calls dm_swap_table, dm_swap_table calls __bind, __bind calls
> ti->type->finalize. Then we go back to do_resume and call dm_resume which
> calls __dm_resume which calls dm_table_resume_targets which calls the
> preresume callback on all the targets. If some of them fails, it returns a
> failure (despite the fact that ti->type->finalize succeeded), if all of
> them succeed, it calls the resume callback on all of them.
>
> So, it seems that the preresume callback provides the guarantee that you
> looking for.
>
> > -Fan
>
> Mikulas

Thanks for the info. I have tested and verified that the preresume() 
hook can also work for our case.

From the source code 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/md/dm-ioctl.c#n1149, 
the whole resume process appears to be:

1. Check if there is a new map for the device. If so, attempt to 
activate the new map using dm_swap_table() (where the finalize() 
callback occurs).

2. Check if the device is suspended. If so, use dm_resume() (where the 
preresume() callback occurs) to resume the device.

3. If a new map is activated, use dm_table_destroy() to destroy the old map.

For our case:

- Using the finalize() callback, the metadata of the dm-verity target 
inside the table is attached to the mapped device every time a new table 
is activated.
- Using the preresume() callback, the same metadata is attached every 
time the device resumes from suspension.

If I understand the code correctly, resuming from suspension is a 
necessary step for loading a new mapping table. Thus, the preresume() 
callback covers all conditions where the finalize() callback would be 
triggered. However, the preresume() callback can also be triggered when 
the device resumes from suspension without loading a new table, in which 
case there is no new metadata in the table to attach to the mapped device.

In the scenario where the finalize() callback succeeds but the 
preresume() callback fails, it seems the device will remain in a 
suspended state, the newly activated table will be kept, and the old 
table will be destroyed, so it seems there is no inconsistency using 
finalize() even preresume() potentially fails.

I believe both the finalize() callback proposed by Mike and the 
preresume() callback suggested by Mikulas can work for our case. I am 
fine with either approach, but I would like to know which one is 
preferred by the maintainers and would appreciate an ACK for the chosen 
approach.

-Fan