On 05/29/2018 07:19 PM, David Sterba wrote:
On Tue, May 29, 2018 at 01:40:34PM +0800, Anand Jain wrote:
v3->v4: As we traverse through the seed device, fs_device gets
updated with
the child seed fs_devices, so make sure we use the same fs_devices
pointer for the mutex_unlock as used for the mutex_lock.
Well, now that I see the change, shouldn't we always hold the
device_list_mutex of the fs_devices that's being processed? Ie. each
time it's switched, the previous is unlocked and new one locked.
No David. That's because we organize seed device under its parent
fs_devices ((fs_devices::seed)::seed)..so on, and they are a local
cloned copy of the original seed fs_devices. So parent's
fs_devices::device_list_mutex lock will suffice.
On the 2nd thought, though theoretically you are right. But practically
there isn't any use case which can benefit by using the intricate
locking as you suggested above.
I don't think it's intricate or complex, just lock the fs_devices that
can be potentially modified in the following loop.
Schematically:
function called with some fs_devices
again:
lock fs_devices->device_list_mutex
foreach device in fs_devices
if ...
fs_devices counters get changed after device
deletion
endfor
if (fs_devices->seed)
unlock fs_devices->device_list_mutex
fs_devices = fs_devices->seed
lock fs_devices->device_list_mutex <-- lock the new one
goto again
endif
unlock fs_devices->device_list_mutex <-- correctly unlock
endfunc
I am following the following method:-
By holding the parent fs_devices (which is also the mounted fs_devices
lock) it would imply to lock its dependent cloned seed fs_devices, as
to reach the cloned seed device, first we need to traverse from the
parent fs_devices.
Locking the parent fs_devices might be the right scheme, but if any
child fs_devices pointer gets passed to a random function that will
change it, then the locking will not protect it.
If its not holding the parent fs_devices lock then it would a bug and
if its already holding the parent lock then child lock isn't necessary
at all.
Also sent the
[DOC] BTRFS Volume operations, Device Lists and Locks all in one page
to have a holistic vewi.
Its not too convincing to me the approach of using the per fs_devices
lock when fs_devices::seed is traversed its not necessary.
Thanks, Anand
This might need a deeper audit how the seeding device is done and maybe
add some helpers that eg. lock the whole chain so all of them are
protected.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
