[PATCH AUTOSEL 4.4 12/37] jbd2: Fix statistics for the number of logged blocks

[PATCH AUTOSEL 4.4 12/37] jbd2: Fix statistics for the number of logged blocks

[Sasha Levin]

From: Jan Kara <jack@suse.cz>

[ Upstream commit 015c6033068208d6227612c878877919f3fcf6b6 ]

jbd2 statistics counting number of blocks logged in a transaction was
wrong. It didn't count the commit block and more importantly it didn't
count revoke descriptor blocks. Make sure these get properly counted.

Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20191105164437.32602-13-jack@suse.cz
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/jbd2/commit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 2d964ce456060..ebbd7d054cabd 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -740,7 +740,6 @@ start_journal_io:
 				submit_bh(WRITE_SYNC, bh);
 			}
 			cond_resched();
-			stats.run.rs_blocks_logged += bufs;
 
 			/* Force a new descriptor to be generated next
                            time round the loop. */
@@ -827,6 +826,7 @@ start_journal_io:
 		if (unlikely(!buffer_uptodate(bh)))
 			err = -EIO;
 		jbd2_unfile_log_bh(bh);
+		stats.run.rs_blocks_logged++;
 
 		/*
 		 * The list contains temporary buffer heads created by
@@ -872,6 +872,7 @@ start_journal_io:
 		BUFFER_TRACE(bh, "ph5: control buffer writeout done: unfile");
 		clear_buffer_jwrite(bh);
 		jbd2_unfile_log_bh(bh);
+		stats.run.rs_blocks_logged++;
 		__brelse(bh);		/* One for getblk */
 		/* AKPM: bforget here */
 	}
@@ -893,6 +894,7 @@ start_journal_io:
 	}
 	if (cbh)
 		err = journal_wait_on_commit_record(journal, cbh);
+	stats.run.rs_blocks_logged++;
 	if (jbd2_has_feature_async_commit(journal) &&
 	    journal->j_flags & JBD2_BARRIER) {
 		blkdev_issue_flush(journal->j_dev, GFP_NOFS, NULL);
-- 
2.20.1

[PATCH AUTOSEL 4.4 27/37] ext4: work around deleting a file with i_nlink == 0 safely

[Sasha Levin]

From: Theodore Ts'o <tytso@mit.edu>

[ Upstream commit c7df4a1ecb8579838ec8c56b2bb6a6716e974f37 ]

If the file system is corrupted such that a file's i_links_count is
too small, then it's possible that when unlinking that file, i_nlink
will already be zero.  Previously we were working around this kind of
corruption by forcing i_nlink to one; but we were doing this before
trying to delete the directory entry --- and if the file system is
corrupted enough that ext4_delete_entry() fails, then we exit with
i_nlink elevated, and this causes the orphan inode list handling to be
FUBAR'ed, such that when we unmount the file system, the orphan inode
list can get corrupted.

A better way to fix this is to simply skip trying to call drop_nlink()
if i_nlink is already zero, thus moving the check to the place where
it makes the most sense.

https://bugzilla.kernel.org/show_bug.cgi?id=205433

Link: https://lore.kernel.org/r/20191112032903.8828-1-tytso@mit.edu
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@kernel.org
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/ext4/namei.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index aa08e129149dc..712bf332e3948 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3040,18 +3040,17 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 	if (IS_DIRSYNC(dir))
 		ext4_handle_sync(handle);
 
-	if (inode->i_nlink == 0) {
-		ext4_warning_inode(inode, "Deleting file '%.*s' with no links",
-				   dentry->d_name.len, dentry->d_name.name);
-		set_nlink(inode, 1);
-	}
 	retval = ext4_delete_entry(handle, dir, de, bh);
 	if (retval)
 		goto end_unlink;
 	dir->i_ctime = dir->i_mtime = ext4_current_time(dir);
 	ext4_update_dx_flag(dir);
 	ext4_mark_inode_dirty(handle, dir);
-	drop_nlink(inode);
+	if (inode->i_nlink == 0)
+		ext4_warning_inode(inode, "Deleting file '%.*s' with no links",
+				   dentry->d_name.len, dentry->d_name.name);
+	else
+		drop_nlink(inode);
 	if (!inode->i_nlink)
 		ext4_orphan_add(handle, inode);
 	inode->i_ctime = ext4_current_time(inode);
-- 
2.20.1

Re: [PATCH AUTOSEL 4.4 27/37] ext4: work around deleting a file with i_nlink == 0 safely

[Theodore Y. Ts'o]

On Wed, Dec 11, 2019 at 10:38:03AM -0500, Sasha Levin wrote:
> From: Theodore Ts'o <tytso@mit.edu>
> 
> [ Upstream commit c7df4a1ecb8579838ec8c56b2bb6a6716e974f37 ]
> 
> If the file system is corrupted such that a file's i_links_count is
> too small, then it's possible that when unlinking that file, i_nlink
> will already be zero.  Previously we were working around this kind of
> corruption by forcing i_nlink to one; but we were doing this before
> trying to delete the directory entry --- and if the file system is
> corrupted enough that ext4_delete_entry() fails, then we exit with
> i_nlink elevated, and this causes the orphan inode list handling to be
> FUBAR'ed, such that when we unmount the file system, the orphan inode
> list can get corrupted.
> 
> A better way to fix this is to simply skip trying to call drop_nlink()
> if i_nlink is already zero, thus moving the check to the place where
> it makes the most sense.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=205433
> 
> Link: https://lore.kernel.org/r/20191112032903.8828-1-tytso@mit.edu
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@kernel.org
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

I'm confused; this was explicitly cc'ed to stable@kernel.org, so why
is your AUTOSEL picking this up?  I would have thought this would get
picked up via the normal stable kernel processes.

Thanks,

						- Ted

Re: [PATCH AUTOSEL 4.4 27/37] ext4: work around deleting a file with i_nlink == 0 safely

[Greg KH]

On Wed, Dec 11, 2019 at 11:19:59AM -0500, Theodore Y. Ts'o wrote:
> On Wed, Dec 11, 2019 at 10:38:03AM -0500, Sasha Levin wrote:
> > From: Theodore Ts'o <tytso@mit.edu>
> > 
> > [ Upstream commit c7df4a1ecb8579838ec8c56b2bb6a6716e974f37 ]
> > 
> > If the file system is corrupted such that a file's i_links_count is
> > too small, then it's possible that when unlinking that file, i_nlink
> > will already be zero.  Previously we were working around this kind of
> > corruption by forcing i_nlink to one; but we were doing this before
> > trying to delete the directory entry --- and if the file system is
> > corrupted enough that ext4_delete_entry() fails, then we exit with
> > i_nlink elevated, and this causes the orphan inode list handling to be
> > FUBAR'ed, such that when we unmount the file system, the orphan inode
> > list can get corrupted.
> > 
> > A better way to fix this is to simply skip trying to call drop_nlink()
> > if i_nlink is already zero, thus moving the check to the place where
> > it makes the most sense.
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=205433
> > 
> > Link: https://lore.kernel.org/r/20191112032903.8828-1-tytso@mit.edu
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Cc: stable@kernel.org
> > Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> > Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> I'm confused; this was explicitly cc'ed to stable@kernel.org, so why
> is your AUTOSEL picking this up?  I would have thought this would get
> picked up via the normal stable kernel processes.

Perhaps because it is still in my queue, I haven't gotten there yet, but
will do so after this next round of -rcs are released.

thanks,

greg k-h

Re: [PATCH AUTOSEL 4.4 27/37] ext4: work around deleting a file with i_nlink == 0 safely

[Sasha Levin]

On Wed, Dec 11, 2019 at 11:19:59AM -0500, Theodore Y. Ts'o wrote:
>On Wed, Dec 11, 2019 at 10:38:03AM -0500, Sasha Levin wrote:
>> From: Theodore Ts'o <tytso@mit.edu>
>>
>> [ Upstream commit c7df4a1ecb8579838ec8c56b2bb6a6716e974f37 ]
>>
>> If the file system is corrupted such that a file's i_links_count is
>> too small, then it's possible that when unlinking that file, i_nlink
>> will already be zero.  Previously we were working around this kind of
>> corruption by forcing i_nlink to one; but we were doing this before
>> trying to delete the directory entry --- and if the file system is
>> corrupted enough that ext4_delete_entry() fails, then we exit with
>> i_nlink elevated, and this causes the orphan inode list handling to be
>> FUBAR'ed, such that when we unmount the file system, the orphan inode
>> list can get corrupted.
>>
>> A better way to fix this is to simply skip trying to call drop_nlink()
>> if i_nlink is already zero, thus moving the check to the place where
>> it makes the most sense.
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=205433
>>
>> Link: https://lore.kernel.org/r/20191112032903.8828-1-tytso@mit.edu
>> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
>> Cc: stable@kernel.org
>> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>
>I'm confused; this was explicitly cc'ed to stable@kernel.org, so why
>is your AUTOSEL picking this up?  I would have thought this would get
>picked up via the normal stable kernel processes.

My mistake, appologies.

-- 
Thanks,
Sasha

Re: [PATCH AUTOSEL 4.4 27/37] ext4: work around deleting a file with i_nlink == 0 safely

[Theodore Y. Ts'o]

On Wed, Dec 11, 2019 at 03:04:54PM -0500, Sasha Levin wrote:
> > I'm confused; this was explicitly cc'ed to stable@kernel.org, so why
> > is your AUTOSEL picking this up?  I would have thought this would get
> > picked up via the normal stable kernel processes.
> 
> My mistake, appologies.

No worries; the intent was that it be backported to stable, and I
don't really care with path it takes.

I just wanted to make sure there wouldn't be confusion if you
backported it to stable, and then Greg tried and then got a merge
conflict.  (Or worse, if the patch was one of the ones where it can be
successfully applied *twice* w/o a patch conflict; I'm not sure if git
cherry-pick is smarter than patch in this regard, but I don't think it
is?)

   	   	      	      	      	    	  - Ted

Re: [PATCH AUTOSEL 4.4 27/37] ext4: work around deleting a file with i_nlink == 0 safely

[Sasha Levin]

On Thu, Dec 12, 2019 at 10:17:06AM -0500, Theodore Y. Ts'o wrote:
>On Wed, Dec 11, 2019 at 03:04:54PM -0500, Sasha Levin wrote:
>> > I'm confused; this was explicitly cc'ed to stable@kernel.org, so why
>> > is your AUTOSEL picking this up?  I would have thought this would get
>> > picked up via the normal stable kernel processes.
>>
>> My mistake, appologies.
>
>No worries; the intent was that it be backported to stable, and I
>don't really care with path it takes.
>
>I just wanted to make sure there wouldn't be confusion if you
>backported it to stable, and then Greg tried and then got a merge
>conflict.  (Or worse, if the patch was one of the ones where it can be
>successfully applied *twice* w/o a patch conflict; I'm not sure if git
>cherry-pick is smarter than patch in this regard, but I don't think it
>is?)

This one was just due to me running a bit faster then usual. I generally
don't filter out stable tagged commits and Greg just gets to them faster
than me (the delay on AUTOSEL is bigger than stable tagged commits).

-- 
Thanks,
Sasha