linux-ext4 - Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD+ocbz0NpXYK9fCxpEYGz6fvWJ_SLw+rYQ2yo3UbKJbbEX8hg@mail.gmail.com>
Date:   Tue, 27 Oct 2020 20:48:27 -0700
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     Ritesh Harjani <riteshh@...ux.ibm.com>
Cc:     Andrea Righi <andrea.righi@...onical.com>,
        "Theodore Ts'o" <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

Actually the simpler fix for this in case of fast commits is to check
if the inode is on the fast commit list or not. Since we clear the
fast commit list after every fast and / or full commit, it's always
true that if the inode is not on the list, that means it isn't dirty.
This will simplify the logic here and then we can probably get rid of
i_fc_committed_subtid field altogether. I'll test this and send out a
patch.

Thanks,
Harshad

On Tue, Oct 27, 2020 at 8:27 PM Ritesh Harjani <riteshh@...ux.ibm.com> wrote:
>
>
>
> On 10/27/20 3:58 AM, harshad shirwadkar wrote:
> > Thanks Andrea for catching and sending out a fix for this.
> >
> > On Sat, Oct 24, 2020 at 7:01 AM Andrea Righi <andrea.righi@...onical.com> wrote:
> >>
> >> ext4_inode_datasync_dirty() needs to return 'true' if the inode is
> >> dirty, 'false' otherwise, but the logic seems to be incorrectly changed
> >> by commit aa75f4d3daae ("ext4: main fast-commit commit path").
> >>
> >> This introduces a problem with swap files that are always failing to be
> >> activated, showing this error in dmesg:
> >>
> >>   [   34.406479] swapon: file is not committed
> >>
>
> Well, I too noticed this yesterday while I was testing xfstests -g swap.
> Those tests were returning _notrun, hence that could be the reason why
> it didn't get notice in XFSTESTing from Ted.
>
> - I did notice that this code was introduced in v10 only.
> This wasn't there in v9 though.
>
>
> >> Simple test case to reproduce the problem:
> >>
> >>    # fallocate -l 8G swapfile
> >>    # chmod 0600 swapfile
> >>    # mkswap swapfile
> >>    # swapon swapfile
> >>
> >> Fix the logic to return the proper state of the inode.
> >>
> >> Link: https://lore.kernel.org/lkml/20201024131333.GA32124@xps-13-7390
> >> Fixes: aa75f4d3daae ("ext4: main fast-commit commit path")
> >> Signed-off-by: Andrea Righi <andrea.righi@...onical.com>
> >> ---
> >>   fs/ext4/inode.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> >> index 03c2253005f0..a890a17ab7e1 100644
> >> --- a/fs/ext4/inode.c
> >> +++ b/fs/ext4/inode.c
> >> @@ -3308,8 +3308,8 @@ static bool ext4_inode_datasync_dirty(struct inode *inode)
> >>          if (journal) {
> >>                  if (jbd2_transaction_committed(journal,
> >>                                          EXT4_I(inode)->i_datasync_tid))
> >> -                       return true;
> >> -               return atomic_read(&EXT4_SB(inode->i_sb)->s_fc_subtid) >=
> >> +                       return false;
> >> +               return atomic_read(&EXT4_SB(inode->i_sb)->s_fc_subtid) <
> >>                          EXT4_I(inode)->i_fc_committed_subtid;
> > In addition, the above condition should only be checked if fast
> > commits are enabled. So, in effect this overall condition will look
> > like this:
> >
> > if (journal) {
> >      if (jbd2_transaction_committed(journal, EXT4_I(inode)->i_datasync_tid))
> >          return false;
> >      if (test_opt2(sb, JOURNAL_FAST_COMMIT))
> >          return atomic_read(&EXT4_SB(inode->i_sb)->s_fc_subtid) <
> > EXT4_I(inode)->i_fc_committed_subtid;
> >      return true;
> > }
>
> Yup - I too had made a similar patch. But then I also noticed that below
> condition will always remain false. Since we never update
> "i_fc_committed_subtid" other than at these 2 places
> (one during init where we set it to 0 and other during ext4_fc_commit()
> where we set it to sbi->s_fc_subtid).
>
> <condition>
> atomic_read(&EXT4_SB(inode->i_sb)->s_fc_subtid <
> EXT4_I(inode)->i_fc_committed_subtid
>
>
> Maybe I need more reading around this.
>
> -ritesh
>
>
>
>
>
> >
> > Thanks,
> > Harshad
> >
> >>          }
> >>
> >> --
> >> 2.27.0
> >>