On 22/11/18 07:20AM, Andreas Dilger wrote:
> 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.
Yes, you are right. I will do the change in the next rev.
-ritesh
