On 2021/11/30 21:59, Christoph Hellwig wrote:
> Please post new versions of a patch in a separate thread from the
> original one.
>
>> +static bool loop_try_update_state_locked(struct loop_device *lo, const int old, const int new)
>
> Pleae avoid the overly long line.
Commit bdc48fa11e46f867 ("checkpatch/coding-style: deprecate 80-column warning")
updated max length from 80 to 100, and Documentation/process/coding-style.rst says
The preferred limit on the length of a single line is 80 columns.
Statements longer than 80 columns should be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and does
not hide information.
. But this case did not increase readability?
>
> Also passing arguments as const is a little weird.
The "const" can be added to non pointer arguments for annotating that
the corresponding argument will not change inside a function.
>
>> {
>> + bool ret = false;
>>
>> + lockdep_assert_held(&lo->lo_mutex);
>> + spin_lock(&loop_validate_spinlock);
>> + if (lo->lo_state == old) {
>> + lo->lo_state = new;
>> + ret = true;
>> }
>> + spin_unlock(&loop_validate_spinlock);
>> + return ret;
>
> But I really wonder what the point of this helper is. IMHO it would be
> much easier to follow if it was open coded in the functions that update
> the state (that is without the loop_try_update_state helper as well).
The point of these helpers is to
annotate that lo->lo_state can be updated only by these helpers
and
make sure that appropriate locks are held for synchronization
. If there is a location doing direct "lo->lo_state = Lo_*;" assignment,
it is likely a sign of a bug.
>
> But going one step further: What is protected by loop_validate_spinlock
> and what is protected by lo->lo_mutex now? Or in other words, if we
> decided the lo_state is protected by loop_validate_spinlock why do we
> even need lo_mutex here now?
loop_validate_spinlock protects lo_state of all loop devices, for
loop_validate_file() needs to traverse on remote loop devices.
By changing/checking lo_state with loop_validate_spinlock held,
f = l->lo_backing_file;
in loop_validate_file() is guaranteed to refer to a stable file
(e.g. __loop_clr_fd() is not in progress).
Currently only __loop_clr_fd() is protected by Lo_rundown, and
loop_change_fd()/loop_configure() will be protected by Lo_binding by this patch.
Since there are functions where dedicated Lo_* is not yet used (e.g.
loop_set_status(), loop_get_status(), lo_simple_ioctl()), these helpers
for now need to also hold lo->lo_mutex for serializing against these functions.
If all functions which hold lo->lo_mutex are updated to use dedicated Lo_*,
we can remove lo->lo_mutex.
