| Message ID | 20201024140115.GA35973@xps-13-7390 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: properly check for dirty state in ext4_inode_datasync_dirty() | expand |
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 >
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 >>
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 > >>
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
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
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
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; }
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(-)