Re: [PATCH 1/2] raid5-cache: update superblock at shutdown/reboot

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

On Fri, Nov 18 2016, Shaohua Li wrote:

> On Thu, Nov 17, 2016 at 04:18:15PM +1100, Neil Brown wrote:
>> On Thu, Nov 17 2016, Shaohua Li wrote:
>> 
>> > Currently raid5-cache update superblock in .quiesce. But since at
>> > shutdown/reboot, .quiesce is called with reconfig mutex locked,
>> > superblock isn't guaranteed to be called in reclaim thread (see
>> > 8e018c21da3). This will make assemble do unnecessary journal recovery.
>> > It doesn't corrupt data but is annoying.  This adds an extra hook to
>> > guarantee journal is flushed to raid disks.  And since this hook is
>> > called before superblock update, this will guarantee we have a uptodate
>> > superblock in shutdown/reboot
>> 
>> Hi.
>> I don't quite follow some of the reasoning here.
>> In particular, the ->stop_writes() that you have implemented
>> does almost exactly the same thing as r5l_quiesce(1).
>> So why not simply call ->quiesce(mddev, 1) in __md_stop_writes()??
>> You probably need to also call ->quiesce(mddev, 0) to keep things
>> balanced.
>
> reboot (md_notify_reboot) doesn't call .quiesce, maybe we should do though. And
> in stop, we hold reconfig_mutex before calling .quiesce. And with commit
> 8e018c21da3, r5l_write_super_and_discard_space tries to hold the reconfig_mutex
> before write super, which it can't hold, so superblock write is skipped. After
> .quiesce we don't write superblock. To fix the shutdown case, we can add a
> superblock write after .quiesce. But I think it's more generic to add a
> ->stop_writes since it will work for the reboot case.

I hadn't quite processed that this was about md_notify_reboot().
I would be very wary of optimizing this code.  It should certainly avoid
data loss, but anything more doesn't belong here.
During a clean shutdown the array should be stopped properly.
md_notify_reboot() is only meant for minimizing damaged caused by a
hasty "reboot -f -n".

A "clean" shutdown currently includes systemd/mdadm.shutdown (in the
mdadm package) running "mdadm --wait-clean --scan".
"mdadm --wait-clean" changes the "safe_mode_delay" so that the array
will become "clean" more quickly.
Possibly we should add something to that to trigger a flush of the
journal, and to wait for the flush to complete.

>
>> Also you have introduced a static mutex (which isn't my favourite sort
>> of thing) without giving any explanation why in the changelog comment.
>> So I cannot easily see if that addition is at all justified.
>
> Now it's possible both the reclaim thread and the thread calling
> __md_stop_writes run into r5l_do_reclaim. The mutex is trying to avoid races.
> I'll add comments there.

It seems to me there is already quite a bit of locking in there... I'll
wait to read the comments though.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]