[PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[Andrea Righi]

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

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@canonical.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;
 	}
 
-- 
2.27.0

Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[harshad shirwadkar]

Thanks Andrea for catching and sending out a fix for this.

On Sat, Oct 24, 2020 at 7:01 AM Andrea Righi <andrea.righi@canonical.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
>
> 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@canonical.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;
}

Thanks,
Harshad

>         }
>
> --
> 2.27.0
>

Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[Ritesh Harjani]

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@canonical.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@canonical.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
>>

Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[harshad shirwadkar]

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@linux.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@canonical.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@canonical.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
> >>

Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[Ritesh Harjani]

On 10/28/20 9:18 AM, harshad shirwadkar wrote:
> 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.

Yes, sounds like a better solution. Thanks!

-ritesh

Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[Theodore Y. Ts'o]

On Wed, Oct 28, 2020 at 08:57:03AM +0530, Ritesh Harjani wrote:
> 
> 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.

Yeah, one of the things I discussed with Harshad is we really need a
test that looks like generic/472, but which is in shared/NNN, and
which unconditionally tries to use swapon for those file systems where
swapfiles are expected to work.  This is actually the second
regression caused by our breaking swapfile support (the other being
the iomap bmap change), which escaped our testing because we didn't
notice that generic/472 was skipped.

(Mental note; perhaps we should have a way of flagging tests that are
skipped when previously they would run in the {kvm,gce}-xfstests
framework.)

						- Ted

Re: [PATCH] ext4: properly check for dirty state in ext4_inode_datasync_dirty()

[Ritesh Harjani]

On 10/28/20 8:59 PM, Theodore Y. Ts'o wrote:
> On Wed, Oct 28, 2020 at 08:57:03AM +0530, Ritesh Harjani wrote:
>>
>> 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.
> 
> Yeah, one of the things I discussed with Harshad is we really need a
> test that looks like generic/472, but which is in shared/NNN, and
> which unconditionally tries to use swapon for those file systems where
> swapfiles are expected to work.  This is actually the second
> regression caused by our breaking swapfile support (the other being
> the iomap bmap change), which escaped our testing because we didn't
> notice that generic/472 was skipped.

Yes, agreed this is second in a row.
So with fast-commit, swap tests returned _not_run, since
swapon syscall returned -EINVAL in _require_scratch_swapfile() itself.
This is due to some old commit in fstests to make swap tests work on
btrfs on both kernels (with and w/o support of swapon in btrfs), it
first checks in _require_scratch_swapfile() to see if swapon even works
or not. Hence it skips to run further if _require_scratch_swapfile()
fails.

Secondly with bmap to iomap interface, I guess it should pass
all tests except for case with fallocate files, which I think is
tests/generic/496. But here too it assumes that if 1st time it fails
with falloc then swapon may not be supported for that fs and hence does
_notrun.

I am actually working on this to make these swap tests return some
definitive pass or failure status. Will be sending some patches soon.
I could use your idea to add a test in shared/NNN for testing swap with
fallocate files for ext4 and xfs (for bmap to iomap ext4 regression
category of tests)

Thanks
-ritesh