On Nov 18, 2022, at 05:37, Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> wrote:
>
> On 22/11/18 04:34AM, Andreas Dilger wrote:
>>> On Nov 7, 2022, at 06:22, Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> wrote:
>>>
>>> f_crashdisk test failed with UNIX_IO_FORCE_BOUNCE=yes due to unbalanced
>>> mutex unlock in below path.
>>>
>>> This patch fixes it.
>>>
>>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
>>> ---
>>> lib/ext2fs/unix_io.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
>>> index e53db333..5b894826 100644
>>> --- a/lib/ext2fs/unix_io.c
>>> +++ b/lib/ext2fs/unix_io.c
>>> @@ -305,7 +305,6 @@ bounce_read:
>>> while (size > 0) {
>>> actual = read(data->dev, data->bounce, align_size);
>>> if (actual != align_size) {
>>> - mutex_unlock(data, BOUNCE_MTX);
>>
>> This patch doesn't show enough context, but AFAIK this is jumping before mutex_down()
>> is called, so this *should* be correct as is?
>
> Thanks for the review, Andreas.
>
> Yeah, the patch diff above is not sufficient since it doesn't share enuf
> context.
> But essentially when "actual" is not equal to "align_size", then in this if
> condition it goes to label "short_read:", which always goto error_unlock,
> where we anyways call mutex_unlock()
>
> Looking at a lot of labels in this function, this definitely looks like
> something which can be cleaned up ("raw_read_blk()").
> I will add that to my list of todos.
You are correct, and it means this code is just not very clear to the reader. I think it
would make more sense to move the "short_read:" label to the end of the code:
actual = read(...);
if (actual != size)
goto error_short_read;
goto success_unlock;
:
actual = read(...);
if (actual != align_size) {
actual = really_read;
buf -= really_read;
size += really_read;
goto error_short_read;
}
:
success_unlock:
mutex_unlock(...);
return 0;
error_short_read:
if (actual < 0) {
retval = errno;
actual = 0;
} else {
retval = EXT2_ET_SHORT_READ;
}
error_unlock:
mutex_unlock(...);
That way the code follows the normal error handling convention and is less likely to be
surprising to the reader.
Cheers, Andreas
