[PATCH v4 0/21] fs: Fix missed inode write during fsync

[PATCH v4 0/21] fs: Fix missed inode write during fsync

[Jan Kara]

Hello,

here is v4 of the patch series which fixes the possibly missing inode write
during fsync(2) for filesystems using generic metadata bh tracking. This
revision contains mostly small fixes and improvements since v3, neither
Sashiko nor Christian found fundamental issues with this approach.

Patches have survived some testing with fstests so they should be reasonably
sound. I'd just like Ogawa to have a look at FAT changes. Similarly as for
other filesystems I've included some cleanups and simplifications these changes
enabled but it would be good to get a confirmation from the maintainer this is
what they want. I could also limit FAT changes to a bare minimum fixing the
data integrity issues.

Changes since v3:
* Add a fix for ext2 lost inode updates for IS_SYNC inodes (independent issue
  found during testing)
* Check I_METADATA_WRITEBACK in simple_fsync_noflush() to avoid skipping inodes
  with pending bh writeout
* Reorder buffer writeback in to first write out metadata blocks and only then
  inodes. It should lead to a more consistent filesystem in case of a crash.
* Added handling of IO errors from metadata writeback for IS_SYNC inodes for
  FAT
* Fix inode writeout during eviction for ext2
* Fix conversion of ext4_nfs_commit_metadata() to work for filesystems with
  a journal
* Fix races when handling of IS_SYNC inodes for UDF by using
  sync_inode_metadata()
* Add ext4 helper for accessing i_metadata_bhs
* Fix some bisectability hazards
* Removed some dead branches after FAT conversion

Changes since v2:
* redesigned the patch set to use new superblock operation
* fixed race of inode freeing with IO error processing in
  mark_buffer_write_io_error() 

Changes since v1:
* Fixed freeing for ext4 dynamically allocated mmb struct
* Optimized tracking of block carrying the inode so that we don't flush it
  unnecessarily on fsync
* Add forgotten check for reclaimed bh to mmb_sync() to avoid NULL ptr deref
* Couple other smaller fixups pointed out by Sashiko

								Honza

Original design explanation:

The series strives to make sure sync_inode_metadata() ->
writeback_single_inode() will end up to properly persisting not only the inode
but also all metadata associated with the inode.  This is beneficial for
several reasons:

1) This removes the need for separate mmb_fsync() implementations. Filesystems
can just use simple_fsync().

2) This makes sure all metadata is written for IS_SYNC / IS_DIRSYNC inodes.
Currently most filesystems just write inode itself.

3) This fixes races when multiple fsyncs race for a while where mmb_sync()
could return before all buffers were really persisted (now I_SYNC state flag
properly serializes everything).

To be able to achieve this we add new .sync_inode_metadata superblock operation
which gets called from __writeback_single_inode() and new I_METADATA_WRITEBACK
state flag (putting everything into .write_inode is possible but it looked too
ugly so I've decided for the new operation).

This scheme with I_METADATA_WRITEBACK state flag also fixes the problem that
when WB_SYNC_NONE writeback happened between write(2) and fsync(2), fsync(2)
would fail to properly persist the inode. Note that this problem isn't specific
to filesystems using the generic metadata bh tracking. This patch set doesn't
fix such filesystems as it is already big enough but the universality of this
problem among simple filesystems is another reason why I've chosen the new
superblock operation instead of trying to somehow hack up metadata bh tracking
infrastructure.

Previous versions:
Link: http://lore.kernel.org/r/20260511115725.28441-1-jack@suse.cz # v1
Link: http://lore.kernel.org/r/20260525085035.12891-1-jack@suse.cz # v2
Link: http://lore.kernel.org/r/20260702175436.12226-1-jack@suse.cz # v3

[PATCH v4 01/21] affs: Drop support for metadata bh tracking

[Jan Kara]

AFFS did all the hard work of tracking metadata bhs dirtied for an inode
but it actually never used this information as affs_file_fsync() just
calls sync_blockdev() to writeback all filesystem metadata bhs. After a
discussion with AFFS maintainer nobody cares about AFFS performance
so let's keep this affs_file_fsync() behavior and just drop all the
pointless tracking from AFFS.

CC: David Sterba <dsterba@suse.com>
Acked-by: David Sterba <dsterba@suse.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/affs/affs.h     |  2 --
 fs/affs/amigaffs.c | 12 ++++++------
 fs/affs/file.c     | 25 +++++++++++--------------
 fs/affs/inode.c    | 13 +++++--------
 fs/affs/namei.c    |  9 ++++-----
 fs/affs/super.c    |  1 -
 6 files changed, 26 insertions(+), 36 deletions(-)

diff --git a/fs/affs/affs.h b/fs/affs/affs.h
index 44a3f69d275f..a7faa91deed5 100644
--- a/fs/affs/affs.h
+++ b/fs/affs/affs.h
@@ -44,7 +44,6 @@ struct affs_inode_info {
 	struct mutex i_link_lock;		/* Protects internal inode access. */
 	struct mutex i_ext_lock;		/* Protects internal inode access. */
 #define i_hash_lock i_ext_lock
-	struct mapping_metadata_bhs i_metadata_bhs;
 	u32	 i_blkcnt;			/* block count */
 	u32	 i_extcnt;			/* extended block count */
 	u32	*i_lc;				/* linear cache of extended blocks */
@@ -152,7 +151,6 @@ extern bool	affs_nofilenametruncate(const struct dentry *dentry);
 extern int	affs_check_name(const unsigned char *name, int len,
 				bool notruncate);
 extern int	affs_copy_name(unsigned char *bstr, struct dentry *dentry);
-struct mapping_metadata_bhs *affs_get_metadata_bhs(struct inode *inode);
 
 /* bitmap. c */
 
diff --git a/fs/affs/amigaffs.c b/fs/affs/amigaffs.c
index bed4fc805e8e..6cc0fc9a4cbf 100644
--- a/fs/affs/amigaffs.c
+++ b/fs/affs/amigaffs.c
@@ -57,7 +57,7 @@ affs_insert_hash(struct inode *dir, struct buffer_head *bh)
 		AFFS_TAIL(sb, dir_bh)->hash_chain = cpu_to_be32(ino);
 
 	affs_adjust_checksum(dir_bh, ino);
-	mmb_mark_buffer_dirty(dir_bh, &AFFS_I(dir)->i_metadata_bhs);
+	mark_buffer_dirty(dir_bh);
 	affs_brelse(dir_bh);
 
 	inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
@@ -100,7 +100,7 @@ affs_remove_hash(struct inode *dir, struct buffer_head *rem_bh)
 			else
 				AFFS_TAIL(sb, bh)->hash_chain = ino;
 			affs_adjust_checksum(bh, be32_to_cpu(ino) - hash_ino);
-			mmb_mark_buffer_dirty(bh, &AFFS_I(dir)->i_metadata_bhs);
+			mark_buffer_dirty(bh);
 			AFFS_TAIL(sb, rem_bh)->parent = 0;
 			retval = 0;
 			break;
@@ -180,7 +180,7 @@ affs_remove_link(struct dentry *dentry)
 			affs_unlock_dir(dir);
 			goto done;
 		}
-		mmb_mark_buffer_dirty(link_bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(link_bh);
 
 		memcpy(AFFS_TAIL(sb, bh)->name, AFFS_TAIL(sb, link_bh)->name, 32);
 		retval = affs_insert_hash(dir, bh);
@@ -188,7 +188,7 @@ affs_remove_link(struct dentry *dentry)
 			affs_unlock_dir(dir);
 			goto done;
 		}
-		mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(bh);
 
 		affs_unlock_dir(dir);
 		iput(dir);
@@ -203,7 +203,7 @@ affs_remove_link(struct dentry *dentry)
 			__be32 ino2 = AFFS_TAIL(sb, link_bh)->link_chain;
 			AFFS_TAIL(sb, bh)->link_chain = ino2;
 			affs_adjust_checksum(bh, be32_to_cpu(ino2) - link_ino);
-			mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+			mark_buffer_dirty(bh);
 			retval = 0;
 			/* Fix the link count, if bh is a normal header block without links */
 			switch (be32_to_cpu(AFFS_TAIL(sb, bh)->stype)) {
@@ -306,7 +306,7 @@ affs_remove_header(struct dentry *dentry)
 	retval = affs_remove_hash(dir, bh);
 	if (retval)
 		goto done_unlock;
-	mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 
 	affs_unlock_dir(dir);
 
diff --git a/fs/affs/file.c b/fs/affs/file.c
index 144b17482d12..23e088a7ed4f 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -140,14 +140,14 @@ affs_alloc_extblock(struct inode *inode, struct buffer_head *bh, u32 ext)
 	AFFS_TAIL(sb, new_bh)->parent = cpu_to_be32(inode->i_ino);
 	affs_fix_checksum(sb, new_bh);
 
-	mmb_mark_buffer_dirty(new_bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(new_bh);
 
 	tmp = be32_to_cpu(AFFS_TAIL(sb, bh)->extension);
 	if (tmp)
 		affs_warning(sb, "alloc_ext", "previous extension set (%x)", tmp);
 	AFFS_TAIL(sb, bh)->extension = cpu_to_be32(blocknr);
 	affs_adjust_checksum(bh, blocknr - tmp);
-	mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 
 	AFFS_I(inode)->i_extcnt++;
 	mark_inode_dirty(inode);
@@ -581,7 +581,7 @@ affs_extent_file_ofs(struct inode *inode, u32 newsize)
 		memset(AFFS_DATA(bh) + boff, 0, tmp);
 		be32_add_cpu(&AFFS_DATA_HEAD(bh)->size, tmp);
 		affs_fix_checksum(sb, bh);
-		mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(bh);
 		size += tmp;
 		bidx++;
 	} else if (bidx) {
@@ -603,7 +603,7 @@ affs_extent_file_ofs(struct inode *inode, u32 newsize)
 		AFFS_DATA_HEAD(bh)->size = cpu_to_be32(tmp);
 		affs_fix_checksum(sb, bh);
 		bh->b_state &= ~(1UL << BH_New);
-		mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(bh);
 		if (prev_bh) {
 			u32 tmp_next = be32_to_cpu(AFFS_DATA_HEAD(prev_bh)->next);
 
@@ -613,8 +613,7 @@ affs_extent_file_ofs(struct inode *inode, u32 newsize)
 					     bidx, tmp_next);
 			AFFS_DATA_HEAD(prev_bh)->next = cpu_to_be32(bh->b_blocknr);
 			affs_adjust_checksum(prev_bh, bh->b_blocknr - tmp_next);
-			mmb_mark_buffer_dirty(prev_bh,
-					      &AFFS_I(inode)->i_metadata_bhs);
+			mark_buffer_dirty(prev_bh);
 			affs_brelse(prev_bh);
 		}
 		size += bsize;
@@ -733,7 +732,7 @@ static int affs_write_end_ofs(const struct kiocb *iocb,
 		AFFS_DATA_HEAD(bh)->size = cpu_to_be32(
 			max(boff + tmp, be32_to_cpu(AFFS_DATA_HEAD(bh)->size)));
 		affs_fix_checksum(sb, bh);
-		mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(bh);
 		written += tmp;
 		from += tmp;
 		bidx++;
@@ -766,13 +765,12 @@ static int affs_write_end_ofs(const struct kiocb *iocb,
 						     bidx, tmp_next);
 				AFFS_DATA_HEAD(prev_bh)->next = cpu_to_be32(bh->b_blocknr);
 				affs_adjust_checksum(prev_bh, bh->b_blocknr - tmp_next);
-				mmb_mark_buffer_dirty(prev_bh,
-					&AFFS_I(inode)->i_metadata_bhs);
+				mark_buffer_dirty(prev_bh);
 			}
 		}
 		affs_brelse(prev_bh);
 		affs_fix_checksum(sb, bh);
-		mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(bh);
 		written += bsize;
 		from += bsize;
 		bidx++;
@@ -801,14 +799,13 @@ static int affs_write_end_ofs(const struct kiocb *iocb,
 						     bidx, tmp_next);
 				AFFS_DATA_HEAD(prev_bh)->next = cpu_to_be32(bh->b_blocknr);
 				affs_adjust_checksum(prev_bh, bh->b_blocknr - tmp_next);
-				mmb_mark_buffer_dirty(prev_bh,
-						&AFFS_I(inode)->i_metadata_bhs);
+				mark_buffer_dirty(prev_bh);
 			}
 		} else if (be32_to_cpu(AFFS_DATA_HEAD(bh)->size) < tmp)
 			AFFS_DATA_HEAD(bh)->size = cpu_to_be32(tmp);
 		affs_brelse(prev_bh);
 		affs_fix_checksum(sb, bh);
-		mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(bh);
 		written += tmp;
 		from += tmp;
 		bidx++;
@@ -945,7 +942,7 @@ affs_truncate(struct inode *inode)
 	}
 	AFFS_TAIL(sb, ext_bh)->extension = 0;
 	affs_fix_checksum(sb, ext_bh);
-	mmb_mark_buffer_dirty(ext_bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(ext_bh);
 	affs_brelse(ext_bh);
 
 	if (inode->i_size) {
diff --git a/fs/affs/inode.c b/fs/affs/inode.c
index 5dd1b016bcb0..d4a3f381c4bc 100644
--- a/fs/affs/inode.c
+++ b/fs/affs/inode.c
@@ -206,7 +206,7 @@ affs_write_inode(struct inode *inode, struct writeback_control *wbc)
 		}
 	}
 	affs_fix_checksum(sb, bh);
-	mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 	affs_brelse(bh);
 	affs_free_prealloc(inode);
 	return 0;
@@ -266,11 +266,8 @@ affs_evict_inode(struct inode *inode)
 	if (!inode->i_nlink) {
 		inode->i_size = 0;
 		affs_truncate(inode);
-	} else {
-		mmb_sync(&AFFS_I(inode)->i_metadata_bhs);
 	}
 
-	mmb_invalidate(&AFFS_I(inode)->i_metadata_bhs);
 	clear_inode(inode);
 	affs_free_prealloc(inode);
 	cache_page = (unsigned long)AFFS_I(inode)->i_lc;
@@ -305,7 +302,7 @@ affs_new_inode(struct inode *dir)
 	bh = affs_getzeroblk(sb, block);
 	if (!bh)
 		goto err_bh;
-	mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 	affs_brelse(bh);
 
 	inode->i_uid     = current_fsuid();
@@ -393,17 +390,17 @@ affs_add_entry(struct inode *dir, struct inode *inode, struct dentry *dentry, s3
 		AFFS_TAIL(sb, bh)->link_chain = chain;
 		AFFS_TAIL(sb, inode_bh)->link_chain = cpu_to_be32(block);
 		affs_adjust_checksum(inode_bh, block - be32_to_cpu(chain));
-		mmb_mark_buffer_dirty(inode_bh, &AFFS_I(inode)->i_metadata_bhs);
+		mark_buffer_dirty(inode_bh);
 		set_nlink(inode, 2);
 		ihold(inode);
 	}
 	affs_fix_checksum(sb, bh);
-	mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 	dentry->d_fsdata = (void *)(long)bh->b_blocknr;
 
 	affs_lock_dir(dir);
 	retval = affs_insert_hash(dir, bh);
-	mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 	affs_unlock_dir(dir);
 	affs_unlock_link(inode);
 
diff --git a/fs/affs/namei.c b/fs/affs/namei.c
index c3c6532da4b0..57d8d755aada 100644
--- a/fs/affs/namei.c
+++ b/fs/affs/namei.c
@@ -373,7 +373,7 @@ affs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	}
 	*p = 0;
 	inode->i_size = i + 1;
-	mmb_mark_buffer_dirty(bh, &AFFS_I(inode)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 	affs_brelse(bh);
 	mark_inode_dirty(inode);
 
@@ -443,8 +443,7 @@ affs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	/* TODO: move it back to old_dir, if error? */
 
 done:
-	mmb_mark_buffer_dirty(bh,
-			&AFFS_I(retval ? old_dir : new_dir)->i_metadata_bhs);
+	mark_buffer_dirty(bh);
 	affs_brelse(bh);
 	return retval;
 }
@@ -497,8 +496,8 @@ affs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	retval = affs_insert_hash(old_dir, bh_new);
 	affs_unlock_dir(old_dir);
 done:
-	mmb_mark_buffer_dirty(bh_old, &AFFS_I(new_dir)->i_metadata_bhs);
-	mmb_mark_buffer_dirty(bh_new, &AFFS_I(old_dir)->i_metadata_bhs);
+	mark_buffer_dirty(bh_old);
+	mark_buffer_dirty(bh_new);
 	affs_brelse(bh_old);
 	affs_brelse(bh_new);
 	return retval;
diff --git a/fs/affs/super.c b/fs/affs/super.c
index b232251aa7bb..0ad5127e9fb8 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -108,7 +108,6 @@ static struct inode *affs_alloc_inode(struct super_block *sb)
 	i->i_lc = NULL;
 	i->i_ext_bh = NULL;
 	i->i_pa_cnt = 0;
-	mmb_init(&i->i_metadata_bhs, &i->vfs_inode.i_data);
 
 	return &i->vfs_inode;
 }
-- 
2.51.0

[PATCH v4 02/21] fs: Fix possible UAF in mark_buffer_write_io_error()

[Jan Kara]

When filesystem is freeing inode it calls mmb_invalidate() which removes
bhs from inode's metadata bh tracking and clears b_mmb for them. However
if the inode is getting deleted, we don't bother with calling mmb_sync()
before and thus these buffers can be under IO and we can be racing with
IO completion handler calling mark_buffer_write_io_error(). This race
can lead to mark_buffer_write_io_error() either hitting NULL pointer
reference or trying to operate on already freed inode. Protect the
mapping handling with RCU to make sure mmb and inode aren't freed before
we are done with them.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 9af5f061a1f8..daaa6614a6d6 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1123,12 +1123,18 @@ EXPORT_SYMBOL(mark_buffer_dirty);
 
 void mark_buffer_write_io_error(struct buffer_head *bh)
 {
+	struct mapping_metadata_bhs *mmb;
+
 	set_buffer_write_io_error(bh);
 	/* FIXME: do we need to set this in both places? */
 	if (bh->b_folio && bh->b_folio->mapping)
 		mapping_set_error(bh->b_folio->mapping, -EIO);
-	if (bh->b_mmb)
-		mapping_set_error(bh->b_mmb->mapping, -EIO);
+	/* Protect us from mmb & inode getting freed while we work on it */
+	rcu_read_lock();
+	mmb = READ_ONCE(bh->b_mmb);
+	if (mmb)
+		mapping_set_error(mmb->mapping, -EIO);
+	rcu_read_unlock();
 }
 EXPORT_SYMBOL(mark_buffer_write_io_error);
 
-- 
2.51.0

[PATCH v4 03/21] fs: Fix missed inode writeback when racing with __writeback_single_inode

[Jan Kara]

When mmb_fsync_noflush() or simple_fsync_noflush() race with another
writeback of the same inode, they can see inode dirty bits are already
clear and skip inode writeback although the racing
__writeback_single_inode() didn't yet get to writing anything. This can
result in fsync(2) returning without properly persisting the inode.

We already have I_SYNC bit for this synchronization and
writeback_single_inode() properly uses it so just fix
mmb_fsync_noflush() and simple_fsync_noflush() to take it into account
as well.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c | 5 +++--
 fs/libfs.c  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index daaa6614a6d6..7e5ad9f4754d 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -655,9 +655,10 @@ int mmb_fsync_noflush(struct file *file, struct mapping_metadata_bhs *mmb,
 
 	if (mmb)
 		ret = mmb_sync(mmb);
-	if (!(inode_state_read_once(inode) & I_DIRTY_ALL))
+	if (!(inode_state_read_once(inode) & (I_DIRTY_ALL | I_SYNC)))
 		goto out;
-	if (datasync && !(inode_state_read_once(inode) & I_DIRTY_DATASYNC))
+	if (datasync &&
+	    !(inode_state_read_once(inode) & (I_DIRTY_DATASYNC | I_SYNC)))
 		goto out;
 
 	err = sync_inode_metadata(inode, 1);
diff --git a/fs/libfs.c b/fs/libfs.c
index 5a0d276379d1..57e5971b6331 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1559,9 +1559,10 @@ int simple_fsync_noflush(struct file *file, loff_t start, loff_t end,
 	if (err)
 		return err;
 
-	if (!(inode_state_read_once(inode) & I_DIRTY_ALL))
+	if (!(inode_state_read_once(inode) & (I_DIRTY_ALL | I_SYNC)))
 		goto out;
-	if (datasync && !(inode_state_read_once(inode) & I_DIRTY_DATASYNC))
+	if (datasync &&
+	    !(inode_state_read_once(inode) & (I_DIRTY_DATASYNC | I_SYNC)))
 		goto out;
 
 	ret = sync_inode_metadata(inode, 1);
-- 
2.51.0

[PATCH v4 04/21] ext4: Allocate mapping_metadata_bhs struct on demand

[Jan Kara]

Currently every ext4 inode gets mapping_metadata_bhs struct although it
is only needed when running without a journal and only for inodes where
any metadata was dirtied. Allocate mapping_metadata_bhs struct on demand
when dirtying the first metadata buffer for the inode.

Acked-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h      | 13 ++++++++++++-
 fs/ext4/ext4_jbd2.c | 25 +++++++++++++++++++++----
 fs/ext4/fsync.c     | 12 ++++++++----
 fs/ext4/inode.c     | 12 ++++++++----
 fs/ext4/super.c     |  9 ++++++---
 5 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b37c136ea3ab..64f8f63f4415 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1151,7 +1151,7 @@ struct ext4_inode_info {
 	struct rw_semaphore i_data_sem;
 	struct inode vfs_inode;
 	struct jbd2_inode *jinode;
-	struct mapping_metadata_bhs i_metadata_bhs;
+	struct mapping_metadata_bhs *i_metadata_bhs;
 
 	/*
 	 * File creation time. Its function is same as that of
@@ -2126,6 +2126,17 @@ static inline bool ext4_inode_orphan_tracked(struct inode *inode)
 		!list_empty(&EXT4_I(inode)->i_orphan);
 }
 
+static inline struct mapping_metadata_bhs *ext4_i_metadata_bhs(
+							struct inode *inode)
+{
+	/*
+	 * i_metadata_bhs is set in ext4_inode_attach_mmb() using cmpxchg().
+	 * We use READ_ONCE when accessing i_metadata_bhs to make sure we get
+	 * consistent view for all accesses.
+	 */
+	return READ_ONCE(EXT4_I(inode)->i_metadata_bhs);
+}
+
 /*
  * Codes for operating systems
  */
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 9a8c225f2753..3b2f8ac20bf8 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -350,6 +350,21 @@ int __ext4_journal_get_create_access(const char *where, unsigned int line,
 	return 0;
 }
 
+static void ext4_inode_attach_mmb(struct inode *inode)
+{
+	struct mapping_metadata_bhs *mmb;
+
+	/*
+	 * It's difficult to handle failure when marking buffer dirty without
+	 * leaving filesystem corrupted
+	 */
+	mmb = kmalloc_obj(*mmb, GFP_NOFS | __GFP_NOFAIL);
+	mmb_init(mmb, &inode->i_data);
+	/* Someone swapped another mmb before us? */
+	if (cmpxchg(&EXT4_I(inode)->i_metadata_bhs, NULL, mmb))
+		kfree(mmb);
+}
+
 int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 				 handle_t *handle, struct inode *inode,
 				 struct buffer_head *bh)
@@ -389,11 +404,13 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 					 err);
 		}
 	} else {
-		if (inode)
-			mmb_mark_buffer_dirty(bh,
-					      &EXT4_I(inode)->i_metadata_bhs);
-		else
+		if (inode) {
+			if (!ext4_i_metadata_bhs(inode))
+				ext4_inode_attach_mmb(inode);
+			mmb_mark_buffer_dirty(bh, ext4_i_metadata_bhs(inode));
+		} else {
 			mark_buffer_dirty(bh);
+		}
 		if (inode && inode_needs_sync(inode)) {
 			sync_dirty_buffer(bh);
 			if (buffer_req(bh) && !buffer_uptodate(bh)) {
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 924726dcc85f..b7ea4433f4be 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -46,6 +46,7 @@
 static int ext4_sync_parent(struct inode *inode)
 {
 	struct dentry *dentry, *next;
+	struct mapping_metadata_bhs *mmb;
 	int ret = 0;
 
 	if (!ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY))
@@ -68,9 +69,12 @@ static int ext4_sync_parent(struct inode *inode)
 		 * through ext4_evict_inode()) and so we are safe to flush
 		 * metadata blocks and the inode.
 		 */
-		ret = mmb_sync(&EXT4_I(inode)->i_metadata_bhs);
-		if (ret)
-			break;
+		mmb = ext4_i_metadata_bhs(inode);
+		if (mmb) {
+			ret = mmb_sync(mmb);
+			if (ret)
+				break;
+		}
 		ret = sync_inode_metadata(inode, 1);
 		if (ret)
 			break;
@@ -89,7 +93,7 @@ static int ext4_fsync_nojournal(struct file *file, loff_t start, loff_t end,
 	};
 	int ret;
 
-	ret = mmb_fsync_noflush(file, &EXT4_I(inode)->i_metadata_bhs,
+	ret = mmb_fsync_noflush(file, ext4_i_metadata_bhs(inode),
 				start, end, datasync);
 	if (ret)
 		return ret;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ce99807c5f5b..e6acef486ee1 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -186,6 +186,8 @@ void ext4_evict_inode(struct inode *inode)
 	if (EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)
 		ext4_evict_ea_inode(inode);
 	if (inode->i_nlink) {
+		struct mapping_metadata_bhs *mmb;
+
 		/*
 		 * If there's dirty page will lead to data loss, user
 		 * could see stale data.
@@ -195,9 +197,9 @@ void ext4_evict_inode(struct inode *inode)
 			ext4_warning_inode(inode, "data will be lost");
 
 		truncate_inode_pages_final(&inode->i_data);
-		/* Avoid mballoc special inode which has no proper iops */
-		if (!EXT4_SB(inode->i_sb)->s_journal)
-			mmb_sync(&EXT4_I(inode)->i_metadata_bhs);
+		mmb = ext4_i_metadata_bhs(inode);
+		if (mmb)
+			mmb_sync(mmb);
 		goto no_delete;
 	}
 
@@ -3452,6 +3454,7 @@ static bool ext4_release_folio(struct folio *folio, gfp_t wait)
 static bool ext4_inode_datasync_dirty(struct inode *inode)
 {
 	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
+	struct mapping_metadata_bhs *mmb;
 
 	if (journal) {
 		if (jbd2_transaction_committed(journal,
@@ -3462,8 +3465,9 @@ static bool ext4_inode_datasync_dirty(struct inode *inode)
 		return true;
 	}
 
+	mmb = ext4_i_metadata_bhs(inode);
 	/* Any metadata buffers to write? */
-	if (mmb_has_buffers(&EXT4_I(inode)->i_metadata_bhs))
+	if (mmb && mmb_has_buffers(mmb))
 		return true;
 	return inode_state_read_once(inode) & I_DIRTY_DATASYNC;
 }
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 245f67d10ded..8671fa1209dd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1430,7 +1430,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
 	INIT_WORK(&ei->i_rsv_conversion_work, ext4_end_io_rsv_work);
 	ext4_fc_init_inode(&ei->vfs_inode);
 	spin_lock_init(&ei->i_fc_lock);
-	mmb_init(&ei->i_metadata_bhs, &ei->vfs_inode.i_data);
+	ei->i_metadata_bhs = NULL;
 #ifdef CONFIG_LOCKDEP
 	lockdep_set_subclass(&ei->i_data_sem, I_DATA_SEM_NORMAL);
 #endif
@@ -1451,6 +1451,7 @@ static int ext4_drop_inode(struct inode *inode)
 static void ext4_free_in_core_inode(struct inode *inode)
 {
 	fscrypt_free_inode(inode);
+	kfree(ext4_i_metadata_bhs(inode));
 	if (!list_empty(&(EXT4_I(inode)->i_fc_list))) {
 		pr_warn("%s: inode %llu still in fc list",
 			__func__, inode->i_ino);
@@ -1529,9 +1530,11 @@ static void destroy_inodecache(void)
 
 void ext4_clear_inode(struct inode *inode)
 {
+	struct mapping_metadata_bhs *mmb = ext4_i_metadata_bhs(inode);
+
 	ext4_fc_del(inode);
-	if (!EXT4_SB(inode->i_sb)->s_journal)
-		mmb_invalidate(&EXT4_I(inode)->i_metadata_bhs);
+	if (mmb)
+		mmb_invalidate(mmb);
 	clear_inode(inode);
 	ext4_discard_preallocations(inode);
 	/*
-- 
2.51.0

[PATCH v4 05/21] fs: Provide way for filesystem to wait for metadata writeback

[Jan Kara]

Currently, inode and in general metadata writeback is handled in a lazy
manner. When inode is dirty, __writeback_single_inode() calls
.write_inode method which for lots of filesystems just copies inode
metadata into the underlying block buffer. Writeback of other metadata
associated with the inode (as well as buffers underlying inodes) is
usually handled completely separately and implicitely during writeback
of block device inode. This is good for efficiency of WB_SYNC_NONE
writeback or sync(2). However it becomes problematic for situations
where we want to make sure inode and its metadata is really persistent
on disk. fsync(2) is the most pronounced example of this and thus we
have grown a special file operation and various helper functions to
assist with this task. However fsync(2) is not the only case, For
example directories with DIRSYNC flag need similar functionality and
current use of sync_inode_metadata() for this task in filesystems
generally misses writeout of necessary metadata.

Furthermore even fsync(2) handling as implemented by simple_fsync() or
similar helpers is racy and can fail to properly persist the inode. The
problem is that WB_SYNC_NONE writeback can copy inode metadata into
underlying buffer and clean inode dirty bits. Following fsync(2) will
see inode is clean and will fail to make sure underlying buffer is
written out.

When multiple fsync(2) calls race, there's also another type of race
involving mmb_fsync(). There the problem is buffers already submitted to
the disk are no longer tracked in the mmb list and so racing mmb_sync()
can return before all of the IO completes.

Provide a new inode state bit I_METADATA_WRITEBACK tracking whether
writeback of inode related metadata may be needed for successful data
integrity sync and if this bit is set __writeback_single_inode() for
data integrity writeback will call new superblock operation
.sync_inode_metadata whose task is to make sure all metadata associated
with the inode (including the inode itself) is properly persisted. This
will allow filesystems to address the data integrity issues described
above and at the same time somewhat simplify our fsync implementations.
Issues with racing fsync(2) calls will be addressed by synchronization on
I_SYNC inode state which is set while calling .sync_inode_metadata, issues
with missed inode buffer writeback are fixed by filesystems looking up
corresponding buffer head and writing it out if needed.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fs-writeback.c              | 33 ++++++++++++++++++++++++++-------
 fs/libfs.c                     |  6 ++++--
 include/linux/fs.h             | 10 +++++++++-
 include/linux/fs/super_types.h |  2 ++
 4 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index fdb8766d275a..9d96357731a3 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1851,6 +1851,22 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 		if (ret == 0)
 			ret = err;
 	}
+
+	/*
+	 * Do we need to wait for inode metadata IO possibly submitted
+	 * by previous WB_SYNC_NONE writeback?
+	 */
+	if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync &&
+	    inode_state_read_once(inode) & I_METADATA_WRITEBACK) {
+		int err;
+
+		spin_lock(&inode->i_lock);
+		inode_state_clear(inode, I_METADATA_WRITEBACK);
+		spin_unlock(&inode->i_lock);
+		err = inode->i_sb->s_op->sync_inode_metadata(inode, wbc);
+		if (ret == 0)
+			ret = err;
+	}
 	wbc->unpinned_netfs_wb = false;
 	trace_writeback_single_inode(inode, wbc, nr_to_write);
 	return ret;
@@ -1892,14 +1908,17 @@ static int writeback_single_inode(struct inode *inode,
 	/*
 	 * If the inode is already fully clean, then there's nothing to do.
 	 *
-	 * For data-integrity syncs we also need to check whether any pages are
-	 * still under writeback, e.g. due to prior WB_SYNC_NONE writeback.  If
-	 * there are any such pages, we'll need to wait for them.
+	 * For data-integrity syncs we also need to check whether any folios or
+	 * metadata are still under writeback, e.g. due to prior WB_SYNC_NONE
+	 * writeback. If there, we'll need to wait for them.
 	 */
-	if (!(inode_state_read(inode) & I_DIRTY_ALL) &&
-	    (wbc->sync_mode != WB_SYNC_ALL ||
-	     !mapping_tagged(inode->i_mapping, PAGECACHE_TAG_WRITEBACK)))
-		goto out;
+	if (!(inode_state_read(inode) & I_DIRTY_ALL)) {
+		if (wbc->sync_mode != WB_SYNC_ALL)
+			goto out;
+		if (!mapping_tagged(inode->i_mapping, PAGECACHE_TAG_WRITEBACK) &&
+		    !(inode_state_read(inode) & I_METADATA_WRITEBACK))
+			goto out;
+	}
 	inode_state_set(inode, I_SYNC);
 	wbc_attach_and_unlock_inode(wbc, inode);
 
diff --git a/fs/libfs.c b/fs/libfs.c
index 57e5971b6331..27d7dc16fcb0 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1559,10 +1559,12 @@ int simple_fsync_noflush(struct file *file, loff_t start, loff_t end,
 	if (err)
 		return err;
 
-	if (!(inode_state_read_once(inode) & (I_DIRTY_ALL | I_SYNC)))
+	if (!(inode_state_read_once(inode) &
+			(I_DIRTY_ALL | I_SYNC | I_METADATA_WRITEBACK)))
 		goto out;
 	if (datasync &&
-	    !(inode_state_read_once(inode) & (I_DIRTY_DATASYNC | I_SYNC)))
+	    !(inode_state_read_once(inode) &
+			(I_DIRTY_DATASYNC | I_SYNC | I_METADATA_WRITEBACK)))
 		goto out;
 
 	ret = sync_inode_metadata(inode, 1);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 50ce731a2b78..729e3cb89e38 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -740,7 +740,8 @@ enum inode_state_flags_enum {
 	I_CREATING		= (1U << 15),
 	I_DONTCACHE		= (1U << 16),
 	I_SYNC_QUEUED		= (1U << 17),
-	I_PINNING_NETFS_WB	= (1U << 18)
+	I_PINNING_NETFS_WB	= (1U << 18),
+	I_METADATA_WRITEBACK	= (1U << 19),
 };
 
 #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
@@ -2213,6 +2214,13 @@ static inline void mark_inode_dirty_sync(struct inode *inode)
 	__mark_inode_dirty(inode, I_DIRTY_SYNC);
 }
 
+static inline void set_inode_metadata_writeback(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	inode_state_set(inode, I_METADATA_WRITEBACK);
+	spin_unlock(&inode->i_lock);
+}
+
 /*
  * returns the refcount on the inode. it can change arbitrarily.
  */
diff --git a/include/linux/fs/super_types.h b/include/linux/fs/super_types.h
index ef7941e9dc79..170561e8f2e2 100644
--- a/include/linux/fs/super_types.h
+++ b/include/linux/fs/super_types.h
@@ -86,6 +86,8 @@ struct super_operations {
 	void (*free_inode)(struct inode *inode);
 	void (*dirty_inode)(struct inode *inode, int flags);
 	int (*write_inode)(struct inode *inode, struct writeback_control *wbc);
+	int (*sync_inode_metadata)(struct inode *inode,
+				   struct writeback_control *wbc);
 	int (*drop_inode)(struct inode *inode);
 	void (*evict_inode)(struct inode *inode);
 	void (*put_super)(struct super_block *sb);
-- 
2.51.0

[PATCH v4 06/21] ext2: Fix lost inode updates for IS_SYNC inodes

[Jan Kara]

ext2_setsize() and ext2_xattr_set2() had a construct like:

	if (IS_SYNC(inode)) {
		sync_inode_metadata(inode, 1);
	} else {
		mark_inode_dirty(inode);
	}

which leads to lost inode updates for IS_SYNC inodes because
sync_inode_metadata() does anything only if the inode is already dirty
and hence inode updates may be simply lost. Fix the problem by
unconditionally marking the inode dirty and *then* call
sync_inode_metadata().

CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/inode.c | 7 ++-----
 fs/ext2/xattr.c | 4 ++--
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 29808629cce5..269b1c9fba5f 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1258,12 +1258,9 @@ static int ext2_setsize(struct inode *inode, loff_t newsize)
 	filemap_invalidate_unlock(inode->i_mapping);
 
 	inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
-	if (inode_needs_sync(inode)) {
-		mmb_sync(&EXT2_I(inode)->i_metadata_bhs);
+	mark_inode_dirty(inode);
+	if (inode_needs_sync(inode))
 		sync_inode_metadata(inode, 1);
-	} else {
-		mark_inode_dirty(inode);
-	}
 
 	return 0;
 }
diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index e55d16abf422..be63f89402a3 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -777,6 +777,7 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
 	/* Update the inode. */
 	EXT2_I(inode)->i_file_acl = new_bh ? new_bh->b_blocknr : 0;
 	inode_set_ctime_current(inode);
+	mark_inode_dirty(inode);
 	if (IS_SYNC(inode)) {
 		error = sync_inode_metadata(inode, 1);
 		/* In case sync failed due to ENOSPC the inode was actually
@@ -789,8 +790,7 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
 			}
 			goto cleanup;
 		}
-	} else
-		mark_inode_dirty(inode);
+	}
 
 	error = 0;
 	if (old_bh && old_bh != new_bh) {
-- 
2.51.0

[PATCH v4 07/21] ext2: Drop __ext2_write_inode()

[Jan Kara]

Fold special helper __ext2_write_inode() into ext2_write_inode() and
just learn the single caller of __ext2_write_inode() to pass proper wbc
instead. No functional changes.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/inode.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 269b1c9fba5f..4dbe52e42d82 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -39,8 +39,6 @@
 #include "acl.h"
 #include "xattr.h"
 
-static int __ext2_write_inode(struct inode *inode, int do_sync);
-
 /*
  * Test whether an inode is a fast symlink.
  */
@@ -83,11 +81,16 @@ void ext2_evict_inode(struct inode * inode)
 	truncate_inode_pages_final(&inode->i_data);
 
 	if (want_delete) {
+		struct writeback_control wbc = {
+			.sync_mode = inode_needs_sync(inode) ? WB_SYNC_ALL :
+							       WB_SYNC_NONE,
+		};
+
 		sb_start_intwrite(inode->i_sb);
 		/* set dtime */
 		EXT2_I(inode)->i_dtime	= ktime_get_real_seconds();
 		mark_inode_dirty(inode);
-		__ext2_write_inode(inode, inode_needs_sync(inode));
+		ext2_write_inode(inode, &wbc);
 		/* truncate to 0 */
 		inode->i_size = 0;
 		if (inode->i_blocks)
@@ -1466,7 +1469,7 @@ struct inode *ext2_iget (struct super_block *sb, unsigned long ino)
 	return ERR_PTR(ret);
 }
 
-static int __ext2_write_inode(struct inode *inode, int do_sync)
+int ext2_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
 	struct ext2_inode_info *ei = EXT2_I(inode);
 	struct super_block *sb = inode->i_sb;
@@ -1557,7 +1560,7 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
 	} else for (n = 0; n < EXT2_N_BLOCKS; n++)
 		raw_inode->i_block[n] = ei->i_data[n];
 	mark_buffer_dirty(bh);
-	if (do_sync) {
+	if (wbc->sync_mode == WB_SYNC_ALL) {
 		sync_dirty_buffer(bh);
 		if (buffer_req(bh) && !buffer_uptodate(bh)) {
 			printk ("IO error syncing ext2 inode [%s:%08lx]\n",
@@ -1570,11 +1573,6 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
 	return err;
 }
 
-int ext2_write_inode(struct inode *inode, struct writeback_control *wbc)
-{
-	return __ext2_write_inode(inode, wbc->sync_mode == WB_SYNC_ALL);
-}
-
 int ext2_getattr(struct mnt_idmap *idmap, const struct path *path,
 		 struct kstat *stat, u32 request_mask, unsigned int query_flags)
 {
-- 
2.51.0

[PATCH v4 08/21] ext2: Avoid unnecessary inode buffer writeback for sync(2)

[Jan Kara]

For sync(2) the generic code calls sync_blockdev_nowait() and later
sync_blockdev() to persist all metadata buffers. Thus there's no need
for ext2_write_inode() to do that which speeds up sync(2) writeback.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/inode.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 4dbe52e42d82..904e70f3140e 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1560,7 +1560,11 @@ int ext2_write_inode(struct inode *inode, struct writeback_control *wbc)
 	} else for (n = 0; n < EXT2_N_BLOCKS; n++)
 		raw_inode->i_block[n] = ei->i_data[n];
 	mark_buffer_dirty(bh);
-	if (wbc->sync_mode == WB_SYNC_ALL) {
+	/*
+	 * For sync(2) the generic code will call sync_blockdev() to write
+	 * all metadata more efficiently.
+	 */
+	if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync) {
 		sync_dirty_buffer(bh);
 		if (buffer_req(bh) && !buffer_uptodate(bh)) {
 			printk ("IO error syncing ext2 inode [%s:%08lx]\n",
-- 
2.51.0

[PATCH v4 09/21] ext2: Fix data integrity writeout issues

[Jan Kara]

Ext2 could fail to properly write out inode on fsync(2) due to races
with WB_SYNC_NONE writeback. Several racing fsyncs could also result in
some fsync returning earlier than all metadata buffers were properly
persisted. Finally DIRSYNC handling was not properly persisting all
inode related metadata.

Fix all these issues by using new .sync_inode_metadata method which
makes sure all inode related metadata is written to disk during any
WB_SYNC_ALL writeback.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/dir.c   |  2 +-
 fs/ext2/ext2.h  |  3 +--
 fs/ext2/file.c  | 17 +----------------
 fs/ext2/inode.c | 43 +++++++++++++++++++++++++------------------
 fs/ext2/super.c |  1 +
 5 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 278d4be8ecbe..e17bbc7598c1 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -734,6 +734,6 @@ const struct file_operations ext2_dir_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= ext2_compat_ioctl,
 #endif
-	.fsync		= ext2_fsync,
+	.fsync		= simple_fsync,
 	.setlease	= generic_setlease,
 };
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 79f7b395258c..5642451bf191 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -735,6 +735,7 @@ extern unsigned long ext2_count_free (struct buffer_head *, unsigned);
 /* inode.c */
 extern struct inode *ext2_iget (struct super_block *, unsigned long);
 extern int ext2_write_inode (struct inode *, struct writeback_control *);
+extern int ext2_sync_inode_metadata(struct inode *, struct writeback_control *);
 extern void ext2_evict_inode(struct inode *);
 void ext2_write_failed(struct address_space *mapping, loff_t to);
 extern int ext2_get_block(struct inode *, sector_t, struct buffer_head *, int);
@@ -772,8 +773,6 @@ extern void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es,
 extern const struct file_operations ext2_dir_operations;
 
 /* file.c */
-extern int ext2_fsync(struct file *file, loff_t start, loff_t end,
-		      int datasync);
 extern const struct inode_operations ext2_file_inode_operations;
 extern const struct file_operations ext2_file_operations;
 
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index 8dca9ec4cacd..b9020df7d89e 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -47,21 +47,6 @@ static int ext2_release_file (struct inode * inode, struct file * filp)
 	return 0;
 }
 
-int ext2_fsync(struct file *file, loff_t start, loff_t end, int datasync)
-{
-	int ret;
-	struct inode *inode = file->f_mapping->host;
-	struct super_block *sb = inode->i_sb;
-
-	ret = mmb_fsync(file, &EXT2_I(inode)->i_metadata_bhs,
-			start, end, datasync);
-	if (ret == -EIO)
-		/* We don't really know where the IO error happened... */
-		ext2_error(sb, __func__,
-			   "detected IO error when writing metadata buffers");
-	return ret;
-}
-
 static ssize_t ext2_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
 	struct file *file = iocb->ki_filp;
@@ -213,7 +198,7 @@ const struct file_operations ext2_file_operations = {
 	.mmap_prepare	= generic_file_mmap_prepare,
 	.open		= ext2_file_open,
 	.release	= ext2_release_file,
-	.fsync		= ext2_fsync,
+	.fsync		= simple_fsync,
 	.get_unmapped_area = thp_get_unmapped_area,
 	.splice_read	= filemap_splice_read,
 	.splice_write	= iter_file_splice_write,
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 904e70f3140e..99acd79a4af2 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -81,16 +81,11 @@ void ext2_evict_inode(struct inode * inode)
 	truncate_inode_pages_final(&inode->i_data);
 
 	if (want_delete) {
-		struct writeback_control wbc = {
-			.sync_mode = inode_needs_sync(inode) ? WB_SYNC_ALL :
-							       WB_SYNC_NONE,
-		};
-
 		sb_start_intwrite(inode->i_sb);
 		/* set dtime */
 		EXT2_I(inode)->i_dtime	= ktime_get_real_seconds();
 		mark_inode_dirty(inode);
-		ext2_write_inode(inode, &wbc);
+		sync_inode_metadata(inode, inode_needs_sync(inode));
 		/* truncate to 0 */
 		inode->i_size = 0;
 		if (inode->i_blocks)
@@ -1560,20 +1555,32 @@ int ext2_write_inode(struct inode *inode, struct writeback_control *wbc)
 	} else for (n = 0; n < EXT2_N_BLOCKS; n++)
 		raw_inode->i_block[n] = ei->i_data[n];
 	mark_buffer_dirty(bh);
-	/*
-	 * For sync(2) the generic code will call sync_blockdev() to write
-	 * all metadata more efficiently.
-	 */
-	if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync) {
-		sync_dirty_buffer(bh);
-		if (buffer_req(bh) && !buffer_uptodate(bh)) {
-			printk ("IO error syncing ext2 inode [%s:%08lx]\n",
-				sb->s_id, (unsigned long) ino);
-			err = -EIO;
-		}
-	}
 	ei->i_state &= ~EXT2_STATE_NEW;
 	brelse (bh);
+	set_inode_metadata_writeback(inode);
+	return err;
+}
+
+int ext2_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
+{
+	struct buffer_head *bh;
+	struct ext2_inode *raw_inode = ext2_get_inode(inode->i_sb, inode->i_ino,
+						      &bh);
+	int err = 0;
+
+	if (IS_ERR(raw_inode))
+		return -EIO;
+	err = mmb_sync(&EXT2_I(inode)->i_metadata_bhs);
+	if (err)
+		goto out;
+	sync_dirty_buffer(bh);
+	if (buffer_write_io_error(bh)) {
+		printk("IO error syncing ext2 inode [%s:%08lx]\n",
+		       inode->i_sb->s_id, (unsigned long)inode->i_ino);
+		err = -EIO;
+	}
+out:
+	brelse(bh);
 	return err;
 }
 
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 3999f8f3b156..a40f530872a4 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -362,6 +362,7 @@ static const struct super_operations ext2_sops = {
 	.alloc_inode	= ext2_alloc_inode,
 	.free_inode	= ext2_free_in_core_inode,
 	.write_inode	= ext2_write_inode,
+	.sync_inode_metadata = ext2_sync_inode_metadata,
 	.evict_inode	= ext2_evict_inode,
 	.put_super	= ext2_put_super,
 	.sync_fs	= ext2_sync_fs,
-- 
2.51.0

[PATCH v4 10/21] udf: Fix data integrity writeout issues

[Jan Kara]

UDF could fail to properly write out inode on fsync(2) due to races
with WB_SYNC_NONE writeback. Several racing fsyncs could also result in
some fsync returning earlier than all metadata buffers were properly
persisted.

Fix all these issues by using new .sync_inode_metadata method which
makes sure all inode related metadata is written to disk during any
WB_SYNC_ALL writeback.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/dir.c     |  2 +-
 fs/udf/file.c    |  9 +--------
 fs/udf/inode.c   | 40 +++++++++++++++++++++++++++++-----------
 fs/udf/super.c   |  1 +
 fs/udf/udfdecl.h |  2 +-
 5 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/fs/udf/dir.c b/fs/udf/dir.c
index ebc9f6a379fe..425e3c162935 100644
--- a/fs/udf/dir.c
+++ b/fs/udf/dir.c
@@ -157,6 +157,6 @@ const struct file_operations udf_dir_operations = {
 	.read			= generic_read_dir,
 	.iterate_shared		= udf_readdir,
 	.unlocked_ioctl		= udf_ioctl,
-	.fsync			= udf_fsync,
+	.fsync			= simple_fsync,
 	.setlease		= generic_setlease,
 };
diff --git a/fs/udf/file.c b/fs/udf/file.c
index f7f1422de30f..0748cc965117 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -198,13 +198,6 @@ static int udf_file_mmap(struct file *file, struct vm_area_struct *vma)
 	return 0;
 }
 
-int udf_fsync(struct file *file, loff_t start, loff_t end, int datasync)
-{
-	return mmb_fsync(file,
-			&UDF_I(file->f_mapping->host)->i_metadata_bhs,
-			start, end, datasync);
-}
-
 const struct file_operations udf_file_operations = {
 	.read_iter		= generic_file_read_iter,
 	.unlocked_ioctl		= udf_ioctl,
@@ -212,7 +205,7 @@ const struct file_operations udf_file_operations = {
 	.mmap			= udf_file_mmap,
 	.write_iter		= udf_file_write_iter,
 	.release		= udf_release_file,
-	.fsync			= udf_fsync,
+	.fsync			= simple_fsync,
 	.splice_read		= filemap_splice_read,
 	.splice_write		= iter_file_splice_write,
 	.llseek			= generic_file_llseek,
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 67bcf83758c8..05e61a65478c 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -142,7 +142,9 @@ void udf_evict_inode(struct inode *inode)
 		if (!inode->i_nlink) {
 			want_delete = 1;
 			udf_setsize(inode, 0);
-			udf_update_inode(inode, IS_SYNC(inode));
+			udf_update_inode(inode, 0);
+			if (IS_SYNC(inode))
+				udf_sync_inode_metadata(inode, NULL);
 		}
 		if (iinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB &&
 		    inode->i_size != iinfo->i_lenExtents) {
@@ -1710,6 +1712,30 @@ int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 	return udf_update_inode(inode, wbc->sync_mode == WB_SYNC_ALL);
 }
 
+int udf_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
+{
+	struct buffer_head *bh;
+	int err = 0;
+
+	bh = sb_getblk(inode->i_sb,
+			udf_get_lb_pblock(inode->i_sb,
+					  &UDF_I(inode)->i_location, 0));
+	if (!bh)
+		return -EIO;
+
+	sync_dirty_buffer(bh);
+	if (buffer_write_io_error(bh)) {
+		udf_warn(inode->i_sb, "IO error syncing udf inode [%08llx]\n",
+			 inode->i_ino);
+		err = -EIO;
+		goto out;
+	}
+	err = mmb_sync(&UDF_I(inode)->i_metadata_bhs);
+out:
+	brelse(bh);
+	return err;
+}
+
 static int udf_sync_inode(struct inode *inode)
 {
 	return udf_update_inode(inode, 1);
@@ -1732,7 +1758,6 @@ static int udf_update_inode(struct inode *inode, int do_sync)
 	uint32_t udfperms;
 	uint16_t icbflags;
 	uint16_t crclen;
-	int err = 0;
 	struct udf_sb_info *sbi = UDF_SB(inode->i_sb);
 	unsigned char blocksize_bits = inode->i_sb->s_blocksize_bits;
 	struct udf_inode_info *iinfo = UDF_I(inode);
@@ -1937,17 +1962,10 @@ static int udf_update_inode(struct inode *inode, int do_sync)
 
 	/* write the data blocks */
 	mark_buffer_dirty(bh);
-	if (do_sync) {
-		sync_dirty_buffer(bh);
-		if (buffer_write_io_error(bh)) {
-			udf_warn(inode->i_sb, "IO error syncing udf inode [%08llx]\n",
-				 inode->i_ino);
-			err = -EIO;
-		}
-	}
 	brelse(bh);
+	set_inode_metadata_writeback(inode);
 
-	return err;
+	return 0;
 }
 
 struct inode *__udf_iget(struct super_block *sb, struct kernel_lb_addr *ino,
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 7b85f5a2b79f..e7e9f2a0d24e 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -211,6 +211,7 @@ static const struct super_operations udf_sb_ops = {
 	.alloc_inode	= udf_alloc_inode,
 	.free_inode	= udf_free_in_core_inode,
 	.write_inode	= udf_write_inode,
+	.sync_inode_metadata = udf_sync_inode_metadata,
 	.evict_inode	= udf_evict_inode,
 	.put_super	= udf_put_super,
 	.sync_fs	= udf_sync_fs,
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 6d951e05c004..86dc2d6a2ef1 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -137,7 +137,6 @@ static inline unsigned int udf_dir_entry_len(struct fileIdentDesc *cfi)
 
 /* file.c */
 extern long udf_ioctl(struct file *, unsigned int, unsigned long);
-int udf_fsync(struct file *file, loff_t start, loff_t end, int datasync);
 
 /* inode.c */
 extern struct inode *__udf_iget(struct super_block *, struct kernel_lb_addr *,
@@ -158,6 +157,7 @@ extern struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
 extern int udf_setsize(struct inode *, loff_t);
 extern void udf_evict_inode(struct inode *);
 extern int udf_write_inode(struct inode *, struct writeback_control *wbc);
+int udf_sync_inode_metadata(struct inode *, struct writeback_control *wbc);
 extern int inode_bmap(struct inode *inode, sector_t block,
 		      struct extent_position *pos, struct kernel_lb_addr *eloc,
 		      uint32_t *elen, sector_t *offset, int8_t *etype);
-- 
2.51.0

[PATCH v4 11/21] udf: Use sync_inode_metadata() to writeout IS_SYNC inode

[Jan Kara]

When setting inode size we directly writeout inode in udf_setsize().
This misses proper writeout of other inode related metadata. Use
sync_inode_metadata() instead and move the flushing to udf_setattr() to
avoid it for udf_evict_inode() where it would be pointless.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c  | 2 ++
 fs/udf/inode.c | 5 +----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 0748cc965117..57d11606a2a7 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -246,6 +246,8 @@ static int udf_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 
 	setattr_copy(&nop_mnt_idmap, inode, attr);
 	mark_inode_dirty(inode);
+	if (IS_SYNC(inode))
+		sync_inode_metadata(inode, 1);
 	return 0;
 }
 
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 05e61a65478c..600705f5edf9 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1328,10 +1328,7 @@ int udf_setsize(struct inode *inode, loff_t newsize)
 	}
 update_time:
 	inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
-	if (IS_SYNC(inode))
-		udf_sync_inode(inode);
-	else
-		mark_inode_dirty(inode);
+	mark_inode_dirty(inode);
 	return err;
 }
 
-- 
2.51.0

[PATCH v4 12/21] udf: Drop udf_sync_inode()

[Jan Kara]

The only place using udf_sync_inode() is now inode_getblk() for flushing
IS_SYNC inodes after write and page_mkwrite allocating blocks. For write
the flushing is actually taken care of by generic_write_sync() so it
isn't needed here. For page_mkwrite it does have effect however none of
the other filesystems seem to bother with flushing IS_SYNC inode on page
fault and properly synchronizing such writeback with standard inode
writeback would be slightly complex due to locking constraints. So just
drop IS_SYNC inode handling from inode_getblk().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 600705f5edf9..c751a02d865c 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -52,7 +52,6 @@ struct udf_map_rq;
 
 static umode_t udf_convert_permissions(struct fileEntry *);
 static int udf_update_inode(struct inode *, int);
-static int udf_sync_inode(struct inode *inode);
 static int udf_alloc_i_data(struct inode *inode, size_t size);
 static int inode_getblk(struct inode *inode, struct udf_map_rq *map);
 static int udf_insert_aext(struct inode *, struct extent_position,
@@ -938,10 +937,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
 	iinfo->i_next_alloc_goal = newblocknum + 1;
 	inode_set_ctime_current(inode);
 
-	if (IS_SYNC(inode))
-		udf_sync_inode(inode);
-	else
-		mark_inode_dirty(inode);
+	mark_inode_dirty(inode);
 	ret = 0;
 out_free:
 	brelse(prev_epos.bh);
@@ -1733,11 +1729,6 @@ int udf_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 	return err;
 }
 
-static int udf_sync_inode(struct inode *inode)
-{
-	return udf_update_inode(inode, 1);
-}
-
 static void udf_adjust_time(struct udf_inode_info *iinfo, struct timespec64 time)
 {
 	if (iinfo->i_crtime.tv_sec > time.tv_sec ||
-- 
2.51.0

[PATCH v4 13/21] udf: Use sync_inode_metadata() in udf_evict_inode()

[Jan Kara]

Instead of opencoding inode update in udf_evict_inode() just use
sync_inode_metadata().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index c751a02d865c..8cf0562f34bb 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -141,9 +141,7 @@ void udf_evict_inode(struct inode *inode)
 		if (!inode->i_nlink) {
 			want_delete = 1;
 			udf_setsize(inode, 0);
-			udf_update_inode(inode, 0);
-			if (IS_SYNC(inode))
-				udf_sync_inode_metadata(inode, NULL);
+			sync_inode_metadata(inode, IS_SYNC(inode));
 		}
 		if (iinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB &&
 		    inode->i_size != iinfo->i_lenExtents) {
-- 
2.51.0

[PATCH v4 14/21] udf: Fold udf_update_inode() into udf_write_inode()

[Jan Kara]

There is no point in udf_update_inode() anymore as it has a single
caller. Just fold udf_update_inode() into it.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 8cf0562f34bb..68c6c2ba8ed1 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -51,7 +51,6 @@
 struct udf_map_rq;
 
 static umode_t udf_convert_permissions(struct fileEntry *);
-static int udf_update_inode(struct inode *, int);
 static int udf_alloc_i_data(struct inode *inode, size_t size);
 static int inode_getblk(struct inode *inode, struct udf_map_rq *map);
 static int udf_insert_aext(struct inode *, struct extent_position,
@@ -1698,11 +1697,6 @@ void udf_update_extra_perms(struct inode *inode, umode_t mode)
 		iinfo->i_extraPerms |= FE_PERM_O_DELETE;
 }
 
-int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
-{
-	return udf_update_inode(inode, wbc->sync_mode == WB_SYNC_ALL);
-}
-
 int udf_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 {
 	struct buffer_head *bh;
@@ -1735,7 +1729,7 @@ static void udf_adjust_time(struct udf_inode_info *iinfo, struct timespec64 time
 		iinfo->i_crtime = time;
 }
 
-static int udf_update_inode(struct inode *inode, int do_sync)
+int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
 	struct buffer_head *bh = NULL;
 	struct fileEntry *fe;
-- 
2.51.0

[PATCH v4 15/21] bfs: Fix data integrity writeout issues

[Jan Kara]

BFS could fail to properly write out inode on fsync(2) due to races with
WB_SYNC_NONE writeback. Several racing fsyncs could also result in some
fsync returning earlier than all metadata buffers were properly
persisted.

Fix all these issues by using new .sync_inode_metadata method which
makes sure all inode related metadata is written to disk during any
WB_SYNC_ALL writeback.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/bfs/dir.c   |  9 +--------
 fs/bfs/inode.c | 30 ++++++++++++++++++++++++------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index 5b40ab09a796..9b37ec4bd89a 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -68,17 +68,10 @@ static int bfs_readdir(struct file *f, struct dir_context *ctx)
 	return 0;
 }
 
-static int bfs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
-{
-	return mmb_fsync(file,
-			&BFS_I(file->f_mapping->host)->i_metadata_bhs,
-			start, end, datasync);
-}
-
 const struct file_operations bfs_dir_operations = {
 	.read		= generic_read_dir,
 	.iterate_shared	= bfs_readdir,
-	.fsync		= bfs_fsync,
+	.fsync		= simple_fsync,
 	.llseek		= generic_file_llseek,
 };
 
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index e41efdd35db9..cacc7eff9c33 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -136,7 +136,6 @@ static int bfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 	unsigned long i_sblock;
 	struct bfs_inode *di;
 	struct buffer_head *bh;
-	int err = 0;
 
 	dprintf("ino=%08x\n", ino);
 
@@ -165,13 +164,31 @@ static int bfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 	di->i_eoffset = cpu_to_le32(i_sblock * BFS_BSIZE + inode->i_size - 1);
 
 	mark_buffer_dirty(bh);
-	if (wbc->sync_mode == WB_SYNC_ALL) {
-		sync_dirty_buffer(bh);
-		if (buffer_req(bh) && !buffer_uptodate(bh))
-			err = -EIO;
-	}
 	brelse(bh);
 	mutex_unlock(&info->bfs_lock);
+	set_inode_metadata_writeback(inode);
+	return 0;
+}
+
+static int bfs_sync_inode_metadata(struct inode *inode,
+				   struct writeback_control *wbc)
+{
+	int err = 0;
+	struct bfs_inode *di;
+	struct buffer_head *bh;
+
+	di = find_inode(inode->i_sb, (u16)inode->ino, &bh);
+	if (IS_ERR(di))
+		return PTR_ERR(di);
+
+	sync_dirty_buffer(bh);
+	if (buffer_write_io_error(bh)) {
+		err = -EIO;
+		goto out;
+	}
+	err = mmb_sync(&BFS_I(inode)->i_metadata_bhs);
+out:
+	brelse(bh);
 	return err;
 }
 
@@ -302,6 +319,7 @@ static const struct super_operations bfs_sops = {
 	.alloc_inode	= bfs_alloc_inode,
 	.free_inode	= bfs_free_inode,
 	.write_inode	= bfs_write_inode,
+	.sync_inode_metadata = bfs_sync_inode_metadata,
 	.evict_inode	= bfs_evict_inode,
 	.put_super	= bfs_put_super,
 	.statfs		= bfs_statfs,
-- 
2.51.0

[PATCH v4 16/21] minix: Fix data integrity writeout issues

[Jan Kara]

Minix could fail to properly write out inode on fsync(2) due to races
with WB_SYNC_NONE writeback. Several racing fsyncs could also result in
some fsync returning earlier than all metadata buffers were properly
persisted. Furthermore DIRSYNC handling missed writing inode related
metadata.

Fix all these issues by using new .sync_inode_metadata method which
makes sure all inode related metadata is written to disk during any
WB_SYNC_ALL writeback.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/minix/dir.c   |  2 +-
 fs/minix/file.c  |  9 +--------
 fs/minix/inode.c | 52 ++++++++++++++++++++++++++++++++----------------
 3 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/fs/minix/dir.c b/fs/minix/dir.c
index 361d26d87d2e..2ca16f849d5a 100644
--- a/fs/minix/dir.c
+++ b/fs/minix/dir.c
@@ -23,7 +23,7 @@ const struct file_operations minix_dir_operations = {
 	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.iterate_shared	= minix_readdir,
-	.fsync		= minix_fsync,
+	.fsync		= simple_fsync,
 };
 
 /*
diff --git a/fs/minix/file.c b/fs/minix/file.c
index 86e5943cd2ff..02aabbdb5dea 100644
--- a/fs/minix/file.c
+++ b/fs/minix/file.c
@@ -10,13 +10,6 @@
 #include <linux/buffer_head.h>
 #include "minix.h"
 
-int minix_fsync(struct file *file, loff_t start, loff_t end, int datasync)
-{
-	return mmb_fsync(file,
-			&minix_i(file->f_mapping->host)->i_metadata_bhs,
-			start, end, datasync);
-}
-
 /*
  * We have mostly NULLs here: the current defaults are OK for
  * the minix filesystem.
@@ -26,7 +19,7 @@ const struct file_operations minix_file_operations = {
 	.read_iter	= generic_file_read_iter,
 	.write_iter	= generic_file_write_iter,
 	.mmap_prepare	= generic_file_mmap_prepare,
-	.fsync		= minix_fsync,
+	.fsync		= simple_fsync,
 	.splice_read	= filemap_splice_read,
 };
 
diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index c30cc590698d..daf83e4ff25c 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -24,6 +24,8 @@
 
 static int minix_write_inode(struct inode *inode,
 		struct writeback_control *wbc);
+static int minix_sync_inode_metadata(struct inode *inode,
+				     struct writeback_control *wbc);
 static int minix_statfs(struct dentry *dentry, struct kstatfs *buf);
 
 void __minix_error_inode(struct inode *inode, const char *function,
@@ -128,6 +130,7 @@ static const struct super_operations minix_sops = {
 	.alloc_inode	= minix_alloc_inode,
 	.free_inode	= minix_free_in_core_inode,
 	.write_inode	= minix_write_inode,
+	.sync_inode_metadata = minix_sync_inode_metadata,
 	.evict_inode	= minix_evict_inode,
 	.put_super	= minix_put_super,
 	.statfs		= minix_statfs,
@@ -630,7 +633,7 @@ struct inode *minix_iget(struct super_block *sb, unsigned long ino)
 /*
  * The minix V1 function to synchronize an inode.
  */
-static struct buffer_head * V1_minix_update_inode(struct inode * inode)
+static int V1_minix_update_inode(struct inode * inode)
 {
 	struct buffer_head * bh;
 	struct minix_inode * raw_inode;
@@ -639,7 +642,7 @@ static struct buffer_head * V1_minix_update_inode(struct inode * inode)
 
 	raw_inode = minix_V1_raw_inode(inode->i_sb, inode->i_ino, &bh);
 	if (!raw_inode)
-		return NULL;
+		return -EIO;
 	raw_inode->i_mode = inode->i_mode;
 	raw_inode->i_uid = fs_high2lowuid(i_uid_read(inode));
 	raw_inode->i_gid = fs_high2lowgid(i_gid_read(inode));
@@ -651,13 +654,15 @@ static struct buffer_head * V1_minix_update_inode(struct inode * inode)
 	else for (i = 0; i < 9; i++)
 		raw_inode->i_zone[i] = minix_inode->u.i1_data[i];
 	mark_buffer_dirty(bh);
-	return bh;
+	brelse(bh);
+	set_inode_metadata_writeback(inode);
+	return 0;
 }
 
 /*
  * The minix V2 function to synchronize an inode.
  */
-static struct buffer_head * V2_minix_update_inode(struct inode * inode)
+static int V2_minix_update_inode(struct inode * inode)
 {
 	struct buffer_head * bh;
 	struct minix2_inode * raw_inode;
@@ -666,7 +671,7 @@ static struct buffer_head * V2_minix_update_inode(struct inode * inode)
 
 	raw_inode = minix_V2_raw_inode(inode->i_sb, inode->i_ino, &bh);
 	if (!raw_inode)
-		return NULL;
+		return -EIO;
 	raw_inode->i_mode = inode->i_mode;
 	raw_inode->i_uid = fs_high2lowuid(i_uid_read(inode));
 	raw_inode->i_gid = fs_high2lowgid(i_gid_read(inode));
@@ -680,29 +685,42 @@ static struct buffer_head * V2_minix_update_inode(struct inode * inode)
 	else for (i = 0; i < 10; i++)
 		raw_inode->i_zone[i] = minix_inode->u.i2_data[i];
 	mark_buffer_dirty(bh);
-	return bh;
+	brelse(bh);
+	set_inode_metadata_writeback(inode);
+	return 0;
 }
 
 static int minix_write_inode(struct inode *inode, struct writeback_control *wbc)
+{
+	if (INODE_VERSION(inode) == MINIX_V1)
+		return V1_minix_update_inode(inode);
+	return V2_minix_update_inode(inode);
+}
+
+static int minix_sync_inode_metadata(struct inode *inode,
+				     struct writeback_control *wbc)
 {
 	int err = 0;
 	struct buffer_head *bh;
+	void *raw_inode;
 
 	if (INODE_VERSION(inode) == MINIX_V1)
-		bh = V1_minix_update_inode(inode);
+		raw_inode = minix_V1_raw_inode(inode->i_sb, inode->i_ino, &bh);
 	else
-		bh = V2_minix_update_inode(inode);
-	if (!bh)
+		raw_inode = minix_V2_raw_inode(inode->i_sb, inode->i_ino, &bh);
+	if (!raw_inode)
 		return -EIO;
-	if (wbc->sync_mode == WB_SYNC_ALL && buffer_dirty(bh)) {
-		sync_dirty_buffer(bh);
-		if (buffer_req(bh) && !buffer_uptodate(bh)) {
-			printk("IO error syncing minix inode [%s:%08llx]\n",
-				inode->i_sb->s_id, inode->i_ino);
-			err = -EIO;
-		}
+	err = mmb_sync(&minix_i(inode)->i_metadata_bhs);
+	if (err)
+		goto out;
+	sync_dirty_buffer(bh);
+	if (buffer_write_io_error(bh)) {
+		printk("IO error syncing minix inode [%s:%08llx]\n",
+			inode->i_sb->s_id, inode->i_ino);
+		err = -EIO;
 	}
-	brelse (bh);
+out:
+	brelse(bh);
 	return err;
 }
 
-- 
2.51.0

[PATCH v4 17/21] ext4: Fix data integrity writeout issues in nojournal mode

[Jan Kara]

Several racing fsyncs on ext4 in nojournal mode could result in some
fsync returning earlier than all metadata buffers were properly
persisted. Also ext4_fsync() in nojournal mode was somewhat inefficient
because it was always writing out the inode regardless whether it was
dirty or not.

Fix these issues by using new .sync_inode_metadata method which makes
sure all inode related metadata is written to disk during any
WB_SYNC_ALL writeback in nojournal mode. This also somewhat simplifies
the nojournal mode fsync handling.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/fsync.c | 28 +++++-----------------------
 fs/ext4/inode.c | 44 +++++++++++++++++++++++++++++---------------
 fs/ext4/super.c |  7 ++++++-
 4 files changed, 41 insertions(+), 39 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 64f8f63f4415..0f06155a35a6 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3166,6 +3166,7 @@ extern struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 	__ext4_iget((sb), (ino), (flags), __func__, __LINE__)
 
 extern int  ext4_write_inode(struct inode *, struct writeback_control *);
+extern int  ext4_sync_inode_metadata(struct inode *, struct writeback_control *);
 extern int  ext4_setattr(struct mnt_idmap *, struct dentry *,
 			 struct iattr *);
 extern u32  ext4_dio_alignment(struct inode *inode);
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index b7ea4433f4be..2999c2cc8fcf 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -46,7 +46,6 @@
 static int ext4_sync_parent(struct inode *inode)
 {
 	struct dentry *dentry, *next;
-	struct mapping_metadata_bhs *mmb;
 	int ret = 0;
 
 	if (!ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY))
@@ -69,12 +68,6 @@ static int ext4_sync_parent(struct inode *inode)
 		 * through ext4_evict_inode()) and so we are safe to flush
 		 * metadata blocks and the inode.
 		 */
-		mmb = ext4_i_metadata_bhs(inode);
-		if (mmb) {
-			ret = mmb_sync(mmb);
-			if (ret)
-				break;
-		}
 		ret = sync_inode_metadata(inode, 1);
 		if (ret)
 			break;
@@ -87,22 +80,11 @@ static int ext4_fsync_nojournal(struct file *file, loff_t start, loff_t end,
 				int datasync, bool *needs_barrier)
 {
 	struct inode *inode = file->f_inode;
-	struct writeback_control wbc = {
-		.sync_mode = WB_SYNC_ALL,
-		.nr_to_write = 0,
-	};
 	int ret;
 
-	ret = mmb_fsync_noflush(file, ext4_i_metadata_bhs(inode),
-				start, end, datasync);
+	ret = sync_inode_metadata(inode, 1);
 	if (ret)
 		return ret;
-
-	/* Force writeout of inode table buffer to disk */
-	ret = ext4_write_inode(inode, &wbc);
-	if (ret)
-		return ret;
-
 	ret = ext4_sync_parent(inode);
 
 	if (test_opt(inode->i_sb, BARRIER))
@@ -160,6 +142,10 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (sb_rdonly(inode->i_sb))
 		goto out;
 
+	ret = file_write_and_wait_range(file, start, end);
+	if (ret)
+		goto out;
+
 	if (!EXT4_SB(inode->i_sb)->s_journal) {
 		ret = ext4_fsync_nojournal(file, start, end, datasync,
 					   &needs_barrier);
@@ -168,10 +154,6 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		goto out;
 	}
 
-	ret = file_write_and_wait_range(file, start, end);
-	if (ret)
-		goto out;
-
 	/*
 	 *  The caller's filemap_fdatawrite()/wait will sync the data.
 	 *  Metadata is in the journal, we wait for proper transaction to
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e6acef486ee1..0c220b535590 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5842,24 +5842,38 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc)
 		err = ext4_fc_commit(EXT4_SB(inode->i_sb)->s_journal,
 						EXT4_I(inode)->i_sync_tid);
 	} else {
-		struct ext4_iloc iloc;
+		set_inode_metadata_writeback(inode);
+	}
+	return err;
+}
 
-		err = __ext4_get_inode_loc_noinmem(inode, &iloc);
+int ext4_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
+{
+	struct ext4_iloc iloc;
+	struct mapping_metadata_bhs *mmb;
+	int err;
+
+	/* We should only get here in nojournal mode */
+	if (WARN_ON_ONCE(EXT4_SB(inode->i_sb)->s_journal))
+		return -EFSCORRUPTED;
+
+	err = __ext4_get_inode_loc_noinmem(inode, &iloc);
+	if (err)
+		return err;
+	mmb = READ_ONCE(EXT4_I(inode)->i_metadata_bhs);
+	if (mmb) {
+		err = mmb_sync(mmb);
 		if (err)
-			return err;
-		/*
-		 * sync(2) will flush the whole buffer cache. No need to do
-		 * it here separately for each inode.
-		 */
-		if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync)
-			sync_dirty_buffer(iloc.bh);
-		if (buffer_req(iloc.bh) && !buffer_uptodate(iloc.bh)) {
-			ext4_error_inode_block(inode, iloc.bh->b_blocknr, EIO,
-					       "IO error syncing inode");
-			err = -EIO;
-		}
-		brelse(iloc.bh);
+			goto out;
+	}
+	sync_dirty_buffer(iloc.bh);
+	if (buffer_write_io_error(iloc.bh)) {
+		ext4_error_inode_block(inode, iloc.bh->b_blocknr, EIO,
+				       "IO error syncing inode");
+		err = -EIO;
 	}
+out:
+	brelse(iloc.bh);
 	return err;
 }
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8671fa1209dd..ae33f5bcb133 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1608,9 +1608,13 @@ static int ext4_nfs_commit_metadata(struct inode *inode)
 	struct writeback_control wbc = {
 		.sync_mode = WB_SYNC_ALL
 	};
+	int ret;
 
 	trace_ext4_nfs_commit_metadata(inode);
-	return ext4_write_inode(inode, &wbc);
+	ret = ext4_write_inode(inode, &wbc);
+	if (!ret && inode_state_read_once(inode) & I_METADATA_WRITEBACK)
+		ret = ext4_sync_inode_metadata(inode, &wbc);
+	return ret;
 }
 
 #ifdef CONFIG_QUOTA
@@ -1667,6 +1671,7 @@ static const struct super_operations ext4_sops = {
 	.free_inode	= ext4_free_in_core_inode,
 	.destroy_inode	= ext4_destroy_inode,
 	.write_inode	= ext4_write_inode,
+	.sync_inode_metadata = ext4_sync_inode_metadata,
 	.dirty_inode	= ext4_dirty_inode,
 	.drop_inode	= ext4_drop_inode,
 	.evict_inode	= ext4_evict_inode,
-- 
2.51.0

[PATCH v4 18/21] fat: Fix missed inode writeback during fsync(2)

[Jan Kara]

FAT could fail to properly write out inode on fsync(2) due to races with
WB_SYNC_NONE writeback. Several racing fsyncs could also result in some
fsync returning earlier than all metadata buffers were properly
persisted.

Fix these issues by using new .sync_inode_metadata method which makes
sure all inode related metadata is written to disk during any
WB_SYNC_ALL writeback. The slight disadvantage of this approach is that
when fsync(2) of an inode races with rename(2) of the inode, the window
during which inode isn't properly persisted becomes wider.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fat/file.c  |  3 +--
 fs/fat/inode.c | 54 ++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/fs/fat/file.c b/fs/fat/file.c
index 37e7049b4c8c..8a7585c25207 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -190,8 +190,7 @@ int fat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
 	struct inode *fat_inode = MSDOS_SB(inode->i_sb)->fat_inode;
 	int err;
 
-	err = mmb_fsync_noflush(filp, &MSDOS_I(inode)->i_metadata_bhs,
-				start, end, datasync);
+	err = simple_fsync_noflush(filp, start, end, datasync);
 	if (err)
 		return err;
 
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 3aa52481ad5c..f6f847ff1b1c 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -623,7 +623,34 @@ struct inode *fat_build_inode(struct super_block *sb,
 
 EXPORT_SYMBOL_GPL(fat_build_inode);
 
-static int __fat_write_inode(struct inode *inode, int wait);
+static int __fat_write_inode(struct inode *inode);
+
+static int fat_sync_inode_metadata(struct inode *inode,
+				   struct writeback_control *wbc)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
+	struct buffer_head *bh;
+	loff_t i_pos;
+	sector_t blocknr;
+	int offset;
+
+	if (inode->i_ino == MSDOS_ROOT_INO)
+		return 0;
+	i_pos = fat_i_pos_read(sbi, inode);
+	if (!i_pos)
+		return 0;
+
+	fat_get_blknr_offset(sbi, i_pos, &blocknr, &offset);
+	bh = sb_find_get_block_nonatomic(inode->i_sb, blocknr);
+	/*
+	 * Buffer present? We leave buffer_dirty check for sync_dirty_buffer()
+	 * for proper synchronization with ongoing IO.
+	 */
+	if (bh && buffer_uptodate(bh))
+		sync_dirty_buffer(bh);
+	brelse(bh);
+	return mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);
+}
 
 static void fat_free_eofblocks(struct inode *inode)
 {
@@ -640,7 +667,7 @@ static void fat_free_eofblocks(struct inode *inode)
 		 * any corruption on the next access to the cluster
 		 * chain for the file.
 		 */
-		err = __fat_write_inode(inode, inode_needs_sync(inode));
+		err = sync_inode_metadata(inode, inode_needs_sync(inode));
 		if (err) {
 			fat_msg(inode->i_sb, KERN_WARNING, "Failed to "
 					"update on disk inode for unused "
@@ -854,7 +881,7 @@ static int fat_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
-static int __fat_write_inode(struct inode *inode, int wait)
+static int __fat_write_inode(struct inode *inode)
 {
 	struct super_block *sb = inode->i_sb;
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
@@ -863,7 +890,7 @@ static int __fat_write_inode(struct inode *inode, int wait)
 	struct timespec64 mtime;
 	loff_t i_pos;
 	sector_t blocknr;
-	int err, offset;
+	int offset;
 
 	if (inode->i_ino == MSDOS_ROOT_INO)
 		return 0;
@@ -907,11 +934,9 @@ static int __fat_write_inode(struct inode *inode, int wait)
 	}
 	spin_unlock(&sbi->inode_hash_lock);
 	mark_buffer_dirty(bh);
-	err = 0;
-	if (wait)
-		err = sync_dirty_buffer(bh);
 	brelse(bh);
-	return err;
+	set_inode_metadata_writeback(inode);
+	return 0;
 }
 
 static int fat_write_inode(struct inode *inode, struct writeback_control *wbc)
@@ -925,14 +950,22 @@ static int fat_write_inode(struct inode *inode, struct writeback_control *wbc)
 		err = fat_clusters_flush(sb);
 		mutex_unlock(&MSDOS_SB(sb)->s_lock);
 	} else
-		err = __fat_write_inode(inode, wbc->sync_mode == WB_SYNC_ALL);
+		err = __fat_write_inode(inode);
 
 	return err;
 }
 
 int fat_sync_inode(struct inode *inode)
 {
-	return __fat_write_inode(inode, 1);
+	int err;
+	struct writeback_control wbc = {
+		.sync_mode = WB_SYNC_ALL,
+	};
+
+	err = __fat_write_inode(inode);
+	if (err)
+		return err;
+	return fat_sync_inode_metadata(inode, &wbc);
 }
 
 EXPORT_SYMBOL_GPL(fat_sync_inode);
@@ -942,6 +975,7 @@ static const struct super_operations fat_sops = {
 	.alloc_inode	= fat_alloc_inode,
 	.free_inode	= fat_free_inode,
 	.write_inode	= fat_write_inode,
+	.sync_inode_metadata = fat_sync_inode_metadata,
 	.evict_inode	= fat_evict_inode,
 	.put_super	= fat_put_super,
 	.statfs		= fat_statfs,
-- 
2.51.0

[PATCH v4 19/21] fat: Remove some superfluous sync_dirty_buffer() calls

[Jan Kara]

Now that fat_sync_inode() ends up writing all metadata buffers
associated with the inode, it isn't necessary to write them out
explicitely using sync_dirty_buffer(). Remove the unnecessary calls. To
maintain proper detection of IO errors when writing buffers, make sure
errors from fat_sync_inode() get propagated to the callers.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fat/dir.c         | 23 +++--------------------
 fs/fat/namei_msdos.c | 28 +++++++++++-----------------
 fs/fat/namei_vfat.c  | 21 ++++++++++++---------
 3 files changed, 26 insertions(+), 46 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index c6cca5d00ffd..60f16154224b 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -1053,11 +1053,7 @@ static int __fat_remove_entries(struct inode *dir, loff_t pos, int nr_slots)
 			nr_slots--;
 		}
 		mmb_mark_buffer_dirty(bh, &MSDOS_I(dir)->i_metadata_bhs);
-		if (IS_DIRSYNC(dir))
-			err = sync_dirty_buffer(bh);
 		brelse(bh);
-		if (err)
-			break;
 
 		/* pos is *next* de's position, so this does `- sizeof(de)' */
 		pos += ((orig_slots - nr_slots) * sizeof(*de)) - sizeof(*de);
@@ -1088,11 +1084,7 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
 		nr_slots--;
 	}
 	mmb_mark_buffer_dirty(bh, &MSDOS_I(dir)->i_metadata_bhs);
-	if (IS_DIRSYNC(dir))
-		err = sync_dirty_buffer(bh);
 	brelse(bh);
-	if (err)
-		return err;
 	inode_inc_iversion(dir);
 
 	if (nr_slots) {
@@ -1109,12 +1101,13 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
 	}
 
 	fat_truncate_time(dir, NULL, FAT_UPDATE_ATIME | FAT_UPDATE_CMTIME);
+	err = 0;
 	if (IS_DIRSYNC(dir))
-		(void)fat_sync_inode(dir);
+		err = fat_sync_inode(dir);
 	else
 		mark_inode_dirty(dir);
 
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL_GPL(fat_remove_entries);
 
@@ -1144,21 +1137,11 @@ static int fat_zeroed_cluster(struct inode *dir, sector_t blknr, int nr_used,
 		n++;
 		blknr++;
 		if (n == nr_bhs) {
-			if (IS_DIRSYNC(dir)) {
-				err = fat_sync_bhs(bhs, n);
-				if (err)
-					goto error;
-			}
 			for (i = 0; i < n; i++)
 				brelse(bhs[i]);
 			n = 0;
 		}
 	}
-	if (IS_DIRSYNC(dir)) {
-		err = fat_sync_bhs(bhs, n);
-		if (err)
-			goto error;
-	}
 	for (i = 0; i < n; i++)
 		brelse(bhs[i]);
 
diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index 0fd2971ad4b1..55095c172f0a 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -253,11 +253,11 @@ static int msdos_add_entry(struct inode *dir, const unsigned char *name,
 
 	fat_truncate_time(dir, ts, FAT_UPDATE_CMTIME);
 	if (IS_DIRSYNC(dir))
-		(void)fat_sync_inode(dir);
+		err = fat_sync_inode(dir);
 	else
 		mark_inode_dirty(dir);
 
-	return 0;
+	return err;
 }
 
 /***** Create a file */
@@ -485,7 +485,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 			inode_inc_iversion(old_dir);
 			fat_truncate_time(old_dir, NULL, FAT_UPDATE_CMTIME);
 			if (IS_DIRSYNC(old_dir))
-				(void)fat_sync_inode(old_dir);
+				err = fat_sync_inode(old_dir);
 			else
 				mark_inode_dirty(old_dir);
 			goto out;
@@ -518,26 +518,21 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 		MSDOS_I(old_inode)->i_attrs |= ATTR_HIDDEN;
 	else
 		MSDOS_I(old_inode)->i_attrs &= ~ATTR_HIDDEN;
-	if (IS_DIRSYNC(new_dir)) {
-		err = fat_sync_inode(old_inode);
-		if (err)
-			goto error_inode;
-	} else
-		mark_inode_dirty(old_inode);
-
 	if (update_dotdot) {
 		fat_set_start(dotdot_de, MSDOS_I(new_dir)->i_logstart);
 		mmb_mark_buffer_dirty(dotdot_bh,
 				      &MSDOS_I(old_inode)->i_metadata_bhs);
-		if (IS_DIRSYNC(new_dir)) {
-			err = sync_dirty_buffer(dotdot_bh);
-			if (err)
-				goto error_dotdot;
-		}
 		drop_nlink(old_dir);
 		if (!new_inode)
 			inc_nlink(new_dir);
 	}
+	if (IS_DIRSYNC(new_dir)) {
+		err = fat_sync_inode(old_inode);
+		if (err)
+			goto error_dotdot;
+	} else
+		mark_inode_dirty(old_inode);
+
 
 	err = fat_remove_entries(old_dir, &old_sinfo);	/* and releases bh */
 	old_sinfo.bh = NULL;
@@ -546,7 +541,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 	inode_inc_iversion(old_dir);
 	fat_truncate_time(old_dir, &ts, FAT_UPDATE_CMTIME);
 	if (IS_DIRSYNC(old_dir))
-		(void)fat_sync_inode(old_dir);
+		err = fat_sync_inode(old_dir);
 	else
 		mark_inode_dirty(old_dir);
 
@@ -571,7 +566,6 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 				      &MSDOS_I(old_inode)->i_metadata_bhs);
 		corrupt |= sync_dirty_buffer(dotdot_bh);
 	}
-error_inode:
 	fat_detach(old_inode);
 	fat_attach(old_inode, old_sinfo.i_pos);
 	MSDOS_I(old_inode)->i_attrs = old_attrs;
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index e909447873e3..e7b7d7c887fa 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -679,7 +679,7 @@ static int vfat_add_entry(struct inode *dir, const struct qstr *qname,
 	/* update timestamp */
 	fat_truncate_time(dir, ts, FAT_UPDATE_CMTIME);
 	if (IS_DIRSYNC(dir))
-		(void)fat_sync_inode(dir);
+		err = fat_sync_inode(dir);
 	else
 		mark_inode_dirty(dir);
 cleanup:
@@ -921,14 +921,14 @@ static int vfat_update_dotdot_de(struct inode *dir, struct inode *inode,
 	return 0;
 }
 
-static void vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts)
+static int vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts)
 {
 	inode_inc_iversion(dir);
 	fat_truncate_time(dir, ts, FAT_UPDATE_CMTIME);
 	if (IS_DIRSYNC(dir))
-		(void)fat_sync_inode(dir);
-	else
-		mark_inode_dirty(dir);
+		return fat_sync_inode(dir);
+	mark_inode_dirty(dir);
+	return 0;
 }
 
 static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
@@ -996,7 +996,7 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	old_sinfo.bh = NULL;
 	if (err)
 		goto error_dotdot;
-	vfat_update_dir_metadata(old_dir, &ts);
+	err = vfat_update_dir_metadata(old_dir, &ts);
 
 	if (new_inode) {
 		drop_nlink(new_inode);
@@ -1123,10 +1123,13 @@ static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry
 			vfat_move_nlink(new_dir, old_dir);
 	}
 
-	vfat_update_dir_metadata(old_dir, &ts);
+	err = vfat_update_dir_metadata(old_dir, &ts);
 	/* if directories are not the same, update new_dir as well */
-	if (old_dir != new_dir)
-		vfat_update_dir_metadata(new_dir, &ts);
+	if (old_dir != new_dir) {
+		int err2 = vfat_update_dir_metadata(new_dir, &ts);
+		if (!err)
+			err = err2;
+	}
 
 out:
 	brelse(old_dotdot_bh);
-- 
2.51.0

[PATCH v4 20/21] fat: Replace fat_sync_inode() with sync_inode_metadata()

[Jan Kara]

Use generic sync_inode_metadata() instead of fat_sync_inode() for
persisting inode metadata changes for DIRSYNC inodes. This slightly
simplifies code and also addresses a theoretical race where
fat_sync_inode() could return before all metadata buffers associated
with the inode were properly written out when racing with fsync(2).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fat/dir.c         |  5 ++---
 fs/fat/fat.h         |  1 -
 fs/fat/file.c        |  6 +++---
 fs/fat/inode.c       | 15 ---------------
 fs/fat/misc.c        |  7 ++++---
 fs/fat/namei_msdos.c | 33 ++++++++++++++++-----------------
 fs/fat/namei_vfat.c  | 19 ++++++++++---------
 7 files changed, 35 insertions(+), 51 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 60f16154224b..916f5c4cdcf3 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -1102,10 +1102,9 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
 
 	fat_truncate_time(dir, NULL, FAT_UPDATE_ATIME | FAT_UPDATE_CMTIME);
 	err = 0;
+	mark_inode_dirty(dir);
 	if (IS_DIRSYNC(dir))
-		err = fat_sync_inode(dir);
-	else
-		mark_inode_dirty(dir);
+		err = sync_inode_metadata(dir, 1);
 
 	return err;
 }
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 99ed9228a677..dcb5ba757073 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -421,7 +421,6 @@ extern void fat_detach(struct inode *inode);
 extern struct inode *fat_iget(struct super_block *sb, loff_t i_pos);
 extern struct inode *fat_build_inode(struct super_block *sb,
 			struct msdos_dir_entry *de, loff_t i_pos);
-extern int fat_sync_inode(struct inode *inode);
 extern int fat_fill_super(struct super_block *sb, struct fs_context *fc,
 			  void (*setup)(struct super_block *));
 extern int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de);
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 8a7585c25207..1c835ca5f21a 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -331,15 +331,15 @@ static int fat_free(struct inode *inode, int skip)
 	}
 	MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
 	fat_truncate_time(inode, NULL, FAT_UPDATE_CMTIME);
+	mark_inode_dirty(inode);
 	if (wait) {
-		err = fat_sync_inode(inode);
+		err = sync_inode_metadata(inode, 1);
 		if (err) {
 			MSDOS_I(inode)->i_start = i_start;
 			MSDOS_I(inode)->i_logstart = i_logstart;
 			return err;
 		}
-	} else
-		mark_inode_dirty(inode);
+	}
 
 	/* Write a new EOF, and get the remaining cluster chain for freeing. */
 	if (skip) {
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index f6f847ff1b1c..e3bb7b4713f2 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -955,21 +955,6 @@ static int fat_write_inode(struct inode *inode, struct writeback_control *wbc)
 	return err;
 }
 
-int fat_sync_inode(struct inode *inode)
-{
-	int err;
-	struct writeback_control wbc = {
-		.sync_mode = WB_SYNC_ALL,
-	};
-
-	err = __fat_write_inode(inode);
-	if (err)
-		return err;
-	return fat_sync_inode_metadata(inode, &wbc);
-}
-
-EXPORT_SYMBOL_GPL(fat_sync_inode);
-
 static int fat_show_options(struct seq_file *m, struct dentry *root);
 static const struct super_operations fat_sops = {
 	.alloc_inode	= fat_alloc_inode,
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index 3027ef53af21..be18f6b5819b 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -146,16 +146,17 @@ int fat_chain_add(struct inode *inode, int new_dclus, int nr_cluster)
 	} else {
 		MSDOS_I(inode)->i_start = new_dclus;
 		MSDOS_I(inode)->i_logstart = new_dclus;
+		mark_inode_dirty(inode);
 		/*
 		 * Since generic_write_sync() synchronizes regular files later,
 		 * we sync here only directories.
 		 */
 		if (S_ISDIR(inode->i_mode) && IS_DIRSYNC(inode)) {
-			ret = fat_sync_inode(inode);
+			ret = sync_inode_metadata(inode, 1);
 			if (ret)
 				return ret;
-		} else
-			mark_inode_dirty(inode);
+		}
+
 	}
 	if (new_fclus != (inode->i_blocks >> (sbi->cluster_bits - 9))) {
 		fat_fs_error_ratelimit(
diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index 55095c172f0a..868ef091048e 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -252,10 +252,9 @@ static int msdos_add_entry(struct inode *dir, const unsigned char *name,
 		return err;
 
 	fat_truncate_time(dir, ts, FAT_UPDATE_CMTIME);
+	mark_inode_dirty(dir);
 	if (IS_DIRSYNC(dir))
-		err = fat_sync_inode(dir);
-	else
-		mark_inode_dirty(dir);
+		err = sync_inode_metadata(dir, 1);
 
 	return err;
 }
@@ -473,21 +472,20 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 				MSDOS_I(old_inode)->i_attrs |= ATTR_HIDDEN;
 			else
 				MSDOS_I(old_inode)->i_attrs &= ~ATTR_HIDDEN;
+			mark_inode_dirty(old_inode);
 			if (IS_DIRSYNC(old_dir)) {
-				err = fat_sync_inode(old_inode);
+				err = sync_inode_metadata(old_inode, 1);
 				if (err) {
 					MSDOS_I(old_inode)->i_attrs = old_attrs;
 					goto out;
 				}
-			} else
-				mark_inode_dirty(old_inode);
+			}
 
 			inode_inc_iversion(old_dir);
 			fat_truncate_time(old_dir, NULL, FAT_UPDATE_CMTIME);
+			mark_inode_dirty(old_dir);
 			if (IS_DIRSYNC(old_dir))
-				err = fat_sync_inode(old_dir);
-			else
-				mark_inode_dirty(old_dir);
+				err = sync_inode_metadata(old_dir, 1);
 			goto out;
 		}
 	}
@@ -526,12 +524,12 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 		if (!new_inode)
 			inc_nlink(new_dir);
 	}
+	mark_inode_dirty(old_inode);
 	if (IS_DIRSYNC(new_dir)) {
-		err = fat_sync_inode(old_inode);
+		err = sync_inode_metadata(old_inode, 1);
 		if (err)
 			goto error_dotdot;
-	} else
-		mark_inode_dirty(old_inode);
+	}
 
 
 	err = fat_remove_entries(old_dir, &old_sinfo);	/* and releases bh */
@@ -540,10 +538,9 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 		goto error_dotdot;
 	inode_inc_iversion(old_dir);
 	fat_truncate_time(old_dir, &ts, FAT_UPDATE_CMTIME);
+	mark_inode_dirty(old_dir);
 	if (IS_DIRSYNC(old_dir))
-		err = fat_sync_inode(old_dir);
-	else
-		mark_inode_dirty(old_dir);
+		err = sync_inode_metadata(old_dir, 1);
 
 	if (new_inode) {
 		drop_nlink(new_inode);
@@ -571,8 +568,10 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 	MSDOS_I(old_inode)->i_attrs = old_attrs;
 	if (new_inode) {
 		fat_attach(new_inode, new_i_pos);
-		if (corrupt)
-			corrupt |= fat_sync_inode(new_inode);
+		if (corrupt) {
+			mark_inode_dirty(new_inode);
+			corrupt |= sync_inode_metadata(new_inode, 1);
+		}
 	} else {
 		/*
 		 * If new entry was not sharing the data cluster, it
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index e7b7d7c887fa..308158895b1d 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -678,10 +678,9 @@ static int vfat_add_entry(struct inode *dir, const struct qstr *qname,
 
 	/* update timestamp */
 	fat_truncate_time(dir, ts, FAT_UPDATE_CMTIME);
+	mark_inode_dirty(dir);
 	if (IS_DIRSYNC(dir))
-		err = fat_sync_inode(dir);
-	else
-		mark_inode_dirty(dir);
+		err = sync_inode_metadata(dir, 1);
 cleanup:
 	kfree(slots);
 	return err;
@@ -904,9 +903,9 @@ static int vfat_get_dotdot_de(struct inode *inode, struct buffer_head **bh,
 
 static int vfat_sync_ipos(struct inode *dir, struct inode *inode)
 {
-	if (IS_DIRSYNC(dir))
-		return fat_sync_inode(inode);
 	mark_inode_dirty(inode);
+	if (IS_DIRSYNC(dir))
+		return sync_inode_metadata(inode, 1);
 	return 0;
 }
 
@@ -925,9 +924,9 @@ static int vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts)
 {
 	inode_inc_iversion(dir);
 	fat_truncate_time(dir, ts, FAT_UPDATE_CMTIME);
-	if (IS_DIRSYNC(dir))
-		return fat_sync_inode(dir);
 	mark_inode_dirty(dir);
+	if (IS_DIRSYNC(dir))
+		return sync_inode_metadata(dir, 1);
 	return 0;
 }
 
@@ -1024,8 +1023,10 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	fat_attach(old_inode, old_sinfo.i_pos);
 	if (new_inode) {
 		fat_attach(new_inode, new_i_pos);
-		if (corrupt)
-			corrupt |= fat_sync_inode(new_inode);
+		if (corrupt) {
+			mark_inode_dirty(new_inode);
+			corrupt |= sync_inode_metadata(new_inode, 1);
+		}
 	} else {
 		/*
 		 * If new entry was not sharing the data cluster, it
-- 
2.51.0

[PATCH v4 21/21] vfs: Remove mmb_fsync()

[Jan Kara]

Now that everybody has been converted from mmb_fsync() (and it's variant
mmb_fsync_noflush()) to simple_fsync(), we can delete these calls.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c                 | 74 -------------------------------------
 include/linux/buffer_head.h |  4 --
 2 files changed, 78 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 7e5ad9f4754d..be8b57a635cd 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -628,80 +628,6 @@ int mmb_sync(struct mapping_metadata_bhs *mmb)
 }
 EXPORT_SYMBOL(mmb_sync);
 
-/**
- * mmb_fsync_noflush - fsync implementation for simple filesystems with
- * 		       metadata buffers list
- *
- * @file:	file to synchronize
- * @mmb:	list of metadata bhs to flush
- * @start:	start offset in bytes
- * @end:	end offset in bytes (inclusive)
- * @datasync:	only synchronize essential metadata if true
- *
- * This is an implementation of the fsync method for simple filesystems which
- * track all non-inode metadata in the buffers list hanging off the @mmb
- * structure.
- */
-int mmb_fsync_noflush(struct file *file, struct mapping_metadata_bhs *mmb,
-		      loff_t start, loff_t end, bool datasync)
-{
-	struct inode *inode = file->f_mapping->host;
-	int err;
-	int ret = 0;
-
-	err = file_write_and_wait_range(file, start, end);
-	if (err)
-		return err;
-
-	if (mmb)
-		ret = mmb_sync(mmb);
-	if (!(inode_state_read_once(inode) & (I_DIRTY_ALL | I_SYNC)))
-		goto out;
-	if (datasync &&
-	    !(inode_state_read_once(inode) & (I_DIRTY_DATASYNC | I_SYNC)))
-		goto out;
-
-	err = sync_inode_metadata(inode, 1);
-	if (ret == 0)
-		ret = err;
-
-out:
-	/* check and advance again to catch errors after syncing out buffers */
-	err = file_check_and_advance_wb_err(file);
-	if (ret == 0)
-		ret = err;
-	return ret;
-}
-EXPORT_SYMBOL(mmb_fsync_noflush);
-
-/**
- * mmb_fsync - fsync implementation for simple filesystems with metadata
- * 	       buffers list
- *
- * @file:	file to synchronize
- * @mmb:	list of metadata bhs to flush
- * @start:	start offset in bytes
- * @end:	end offset in bytes (inclusive)
- * @datasync:	only synchronize essential metadata if true
- *
- * This is an implementation of the fsync method for simple filesystems which
- * track all non-inode metadata in the buffers list hanging off the @mmb
- * structure. This also makes sure that a device cache flush operation is
- * called at the end.
- */
-int mmb_fsync(struct file *file, struct mapping_metadata_bhs *mmb,
-	      loff_t start, loff_t end, bool datasync)
-{
-	struct inode *inode = file->f_mapping->host;
-	int ret;
-
-	ret = mmb_fsync_noflush(file, mmb, start, end, datasync);
-	if (!ret)
-		ret = blkdev_issue_flush(inode->i_sb->s_bdev);
-	return ret;
-}
-EXPORT_SYMBOL(mmb_fsync);
-
 /*
  * Called when we've recently written block `bblock', and it is known that
  * `bblock' was for a buffer_boundary() buffer.  This means that the block at
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 8b23bc9a244c..fd2c7115c054 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -210,10 +210,6 @@ void bh_end_async_write(struct bio *bio);
 
 /* Things to do with metadata buffers list */
 void mmb_mark_buffer_dirty(struct buffer_head *bh, struct mapping_metadata_bhs *mmb);
-int mmb_fsync_noflush(struct file *file, struct mapping_metadata_bhs *mmb,
-		      loff_t start, loff_t end, bool datasync);
-int mmb_fsync(struct file *file, struct mapping_metadata_bhs *mmb,
-	      loff_t start, loff_t end, bool datasync);
 void clean_bdev_aliases(struct block_device *bdev, sector_t block,
 			sector_t len);
 static inline void clean_bdev_bh_alias(struct buffer_head *bh)
-- 
2.51.0