Re: [PATCH 1/9] vfs: add __iterate_supers() helper

[Fernando Luis Vazquez Cao <fernando_b1@lab.ntt.co.jp>]

On 2012/09/14 11:36, Eric Sandeen wrote:
> On 9/13/12 6:00 AM, Fernando Luis Vázquez Cao wrote:
>> iterate_supers() calls a function provided by the caller with the s_umount
>> semaphore taken in read mode. However, there may be cases where write mode
>> is preferable, so we add a new helper, __iterate_supers(), which lets one
>> specify the mode of the lock.
>>
>> This will be used to fix the emergency thaw (filesystem unfreeze) code, which
>> iterates over the list of superblocks but needs to hold the s_umount semaphore
>> in _write_ mode bebore carrying out the actual thaw operation.
> I'll go ahead & comment on this even though all of this is to support the
> emergency thaw misfeature which I'm liking less and less this evening.
>
> I think this split is a little odd, usually __foo() provides some fundamental
> action, and foo_*() functions call it after possibly defining or altering aspects of
> that action, or preparing for that action.
>
> So having iterate_supers() do read locks, and __iterate_supers do either read
> or write, seems asymmetric, and isn't very well self-documenting.
>
> Why not have i.e. iterate_supers_read() and iterate_supers_write(), each
> of which can call __iterate_supers(blah, blah, locktype).
>
> Anyway, it'd mean changing 5 callers but it's a little clearer and more symmetric
> to me.  What do you think?
>
> The logic seems fine, just a question about the style.

I agree with you. I will fix the style as you suggest.

Thanks,
Fernando
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html