Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown <neilb@suse.com>]

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

On Wed, Aug 03 2016, NeilBrown wrote:

> [ Unknown signature status ]
> On Sun, Jul 31 2016, shli@kernel.org wrote:
>
>> From: Shaohua Li <shli@fb.com>
>>
>> .quiesce is called with mddev lock hold at most places. There are few
>> exceptions. Calling .quesce without the lock hold could create races. For
>> example, the .quesce of raid1 can't be recursively. The purpose of the patches
>> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
>> superblock and should be called with mddev lock hold.
>>
>> Cc: NeilBrown <neilb@suse.com>
>> Signed-off-by: Shaohua Li <shli@fb.com>
>
> Acked-by: NeilBrown <neilb@suse.com>
>
> This should be safe but I'm not sure I really like it.
> The raid1 quiesce could be changed so that it can be called recursively.
> The raid5-cache situation would be harder to get right and maybe this is
> the best solution... It's just that 'quiesce' should be a fairly
> light-weight operation, just waiting for pending requests to flush.  It
> shouldn't really *need* a lock.

Actually, the more I think about this, the less I like it.

I would much rather make .quiesce lighter weight so that no locking was
needed.

For r5l_quiesce, that probable means removed the "r5l_do_reclaim()".
Stopping and restarting the reclaim thread seems reasonable, but calling
r5l_do_reclaim() should not be needed.  It should be done periodically
by the thread, and at 'stop' time, but otherwise isn't needed.
You would need to hold some mutex while calling md_register_thread, but
that could be probably be log->io_mutex, or maybe even some other new
mutex

Could you explore following that path instead?

Thanks,
NeilBrown

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