Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 19 Nov 2022 09:16:41 +0530
From:   "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     Theodore Ts'o <tytso@mit.edu>, linux-ext4@vger.kernel.org,
        Harshad Shirwadkar <harshadshirwadkar@gmail.com>,
        Wang Shilong <wshilong@ddn.com>,
        Andreas Dilger <adilger.kernel@dilger.ca>, Li Xi <lixi@ddn.com>
Subject: Re: [RFCv1 01/72] e2fsck: Fix unbalanced mutex unlock for BOUNCE_MTX
Message-ID: <20221119034641.umvdsnwlhykb3gpv@riteshh-domain>
References: <20221118113711.qby7gtky5k36f7vd@riteshh-domain>
 <1BEDD834-2D4A-4E8E-936C-90DB5E322F9C@dilger.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1BEDD834-2D4A-4E8E-936C-90DB5E322F9C@dilger.ca>
Precedence: bulk

On 22/11/18 07:20AM, Andreas Dilger wrote:
> On Nov 18, 2022, at 05:37, Ritesh Harjani (IBM) <ritesh.list@gmail.com> wrote:
> > 
> > ﻿On 22/11/18 04:34AM, Andreas Dilger wrote:
> >>> On Nov 7, 2022, at 06:22, Ritesh Harjani (IBM) <ritesh.list@gmail.com> 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@gmail.com>
> >>> ---
> >>> 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