[PATCH 0/2] hfs/hfsplus: don't take inode_lock in ->fsync()

[PATCH 0/2] hfs/hfsplus: don't take inode_lock in ->fsync()

[Viacheslav Dubeyko]

Both hfs_file_fsync() and hfsplus_file_fsync() take inode_lock()
around the metadata part of fsync. For both file systems
it is not needed: the inode's metadata is written by
the same ->write_inode() path that the flusher threads use
without inode_lock, the b-tree records are updated under the b-tree
locks, and the b-tree, bitmap and MDB/volume header flushes are
per-superblock and serialized by their own locks (mdb_lock in HFS,
vh_mutex/alloc_mutex in HFS+). The lock only holds off writers of the
file for the whole duration of the metadata and device cache flushes.

Drop inode_lock()/inode_unlock() from both ->fsync()
handlers. Patch 1 is for HFS+, patch 2 does the same for HFS.

Viacheslav Dubeyko (2):
  hfsplus: don't take inode_lock in hfsplus_file_fsync()
  hfs: don't take inode_lock in hfs_file_fsync()

 fs/hfs/inode.c     | 2 --
 fs/hfsplus/inode.c | 3 ---
 2 files changed, 5 deletions(-)

-- 
2.53.0

[PATCH 1/2] hfsplus: don't take inode_lock in hfsplus_file_fsync()

[Viacheslav Dubeyko]

hfsplus_file_fsync() takes inode_lock() around the metadata part of
fsync. For HFS+ it is not needed, because everything done under the lock is
already serialized by other means:

- sync_inode_metadata() ends up in hfsplus_write_inode(), which is
  also called by the flusher threads and by sync without holding
  inode_lock. Concurrent writeback of the same inode is serialized by
  the writeback code itself (I_SYNC), the extent record is written
  under hip->extents_lock, and the catalog record is updated under
  the catalog b-tree lock taken by hfs_find_init().

- The catalog, extents, attributes and allocation file flushes are
  per-superblock, not per-inode. They are guarded by the atomic
  HFSPLUS_I_*_DIRTY bits and the page cache of the b-tree inodes, so
  fsync() calls on different inodes already run this code in parallel,
  as does hfsplus_sync_fs(), which does the same flushes without
  inode_lock.

- The volume header is prepared and committed under sbi->vh_mutex
  (and sbi->alloc_mutex in hfsplus_commit_superblock()).

So inode_lock only serializes fsync() of an inode against write(),
truncate and setattr on the same inode, and against another fsync()
of it, without protecting any state that is not already protected.
Taking it there also means that fsync() of a large file holds off
writers of that file for the whole duration of the b-tree flushes and
the device cache flush. Other file systems (for example, ext4) don't
take inode_lock in ->fsync() either. Drop inode_lock()/inode_unlock()
from hfsplus_file_fsync().

Fixes: 02c24a82187d ("fs: push i_mutex and filemap_write_and_wait down into ->fsync() handlers")
cc: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
cc: Yangtao Li <frank.li@vivo.com>
cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
---
 fs/hfsplus/inode.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 5fa72b465922..8073fb99217d 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -370,7 +370,6 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 	error = file_write_and_wait_range(file, start, end);
 	if (error)
 		return error;
-	inode_lock(inode);
 
 	/*
 	 * Sync inode metadata into the catalog and extent trees.
@@ -429,8 +428,6 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 	if (!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
 		blkdev_issue_flush(inode->i_sb->s_bdev);
 
-	inode_unlock(inode);
-
 	return error;
 }
 
-- 
2.53.0

[PATCH 2/2] hfs: don't take inode_lock in hfs_file_fsync()

[Viacheslav Dubeyko]

hfs_file_fsync() takes inode_lock() around the metadata part of fsync.
For HFS it is not needed, because everything done under the lock is
already serialized by other means:

- write_inode_now() ends up in hfs_write_inode(), which is also called
  by the flusher threads and by sync without holding inode_lock.
  Concurrent writeback of the same inode is serialized by the
  writeback code itself (I_SYNC), and the extent and catalog records
  are updated under the extents and catalog b-tree locks taken by
  hfs_find_init().

- flush_delayed_work() of mdb_work runs flush_mdb(), which commits the
  MDB, the alternate MDB and the volume bitmap under sbi->mdb_lock.
  This state is per-superblock, so inode_lock of one file never
  protected it anyway.

- sync_blockdev() works on the block device and needs no inode lock.

So inode_lock only serializes fsync() of an inode against write(),
truncate and setattr on the same inode, and against another fsync() of
it, without protecting any state that is not already protected, while
holding off writers of that file for the whole duration of the MDB
commit and the block device flush. Drop inode_lock()/inode_unlock()
from hfs_file_fsync().

Fixes: 02c24a82187d ("fs: push i_mutex and filemap_write_and_wait down into ->fsync() handlers")
cc: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
cc: Yangtao Li <frank.li@vivo.com>
cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
---
 fs/hfs/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index ef1275bbbcbb..f1122ffae712 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -773,7 +773,6 @@ static int hfs_file_fsync(struct file *filp, loff_t start, loff_t end,
 	ret = file_write_and_wait_range(filp, start, end);
 	if (ret)
 		return ret;
-	inode_lock(inode);
 
 	/* sync the inode to buffers */
 	ret = write_inode_now(inode, 0);
@@ -785,7 +784,6 @@ static int hfs_file_fsync(struct file *filp, loff_t start, loff_t end,
 	err = sync_blockdev(sb->s_bdev);
 	if (!ret)
 		ret = err;
-	inode_unlock(inode);
 	return ret;
 }
 
-- 
2.53.0