diff mbox series

[RFCv3,02/10] libfs: Add __generic_file_fsync_nolock implementation

Message ID e65768eb0fe145c803ba4afdc869a1757d51d758.1681365596.git.ritesh.list@gmail.com
State Not Applicable
Headers show
Series ext2: DIO to use iomap | expand

Commit Message

Ritesh Harjani (IBM) April 13, 2023, 8:40 a.m. UTC 
Some of the higher layers like iomap takes inode_lock() when calling
generic_write_sync().
Also writeback already happens from other paths without inode lock,
so it's difficult to say that we really need sync_mapping_buffers() to
take any inode locking here. Having said that, let's add a _nolock
variant of this function in libfs for now so that filesystems like
ext2 and ext4's nojournal mode can use it.

Ext4 when got converted to iomap for direct-io already copied it's own
variant of __generic_file_fsync() without lock. Hence let's add a helper
API and use it both in ext2 and ext4.

Later we can review other filesystems as well to see if we can make
_nolock as the default path if inode_lock() is not necessary here.

Tested-by: Disha Goel <disgoel@linux.ibm.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/libfs.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 45 insertions(+)

Comments

Christoph Hellwig April 14, 2023, 5:59 a.m. UTC | #1 
Still no fan of the naming and placement here.  This is specific
to the fs/buffer.c infrastructure.
Jan Kara April 14, 2023, 12:51 p.m. UTC | #2 
On Thu 13-04-23 22:59:24, Christoph Hellwig wrote:
> Still no fan of the naming and placement here.  This is specific
> to the fs/buffer.c infrastructure.

I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and
creating the new function there if it makes you happier. But I think
function names should be consistent (hence the new function would be named
__generic_file_fsync_nolock()). I agree the name is not ideal and would use
cleanup (along with transitioning everybody to not take i_rwsem) but I
don't want to complicate this series by touching 13+ callsites of
generic_file_fsync() and __generic_file_fsync(). That's for a separate
series.

								Honza
Christoph Hellwig April 14, 2023, 1:12 p.m. UTC | #3 
On Fri, Apr 14, 2023 at 02:51:48PM +0200, Jan Kara wrote:
> On Thu 13-04-23 22:59:24, Christoph Hellwig wrote:
> > Still no fan of the naming and placement here.  This is specific
> > to the fs/buffer.c infrastructure.
> 
> I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and
> creating the new function there if it makes you happier. But I think
> function names should be consistent (hence the new function would be named
> __generic_file_fsync_nolock()). I agree the name is not ideal and would use
> cleanup (along with transitioning everybody to not take i_rwsem) but I
> don't want to complicate this series by touching 13+ callsites of
> generic_file_fsync() and __generic_file_fsync(). That's for a separate
> series.

I would not change the existing function.  Just do the right thing for
the new helper and slowly migrate over without complicating this series.
Jan Kara April 14, 2023, 2:20 p.m. UTC | #4 
On Fri 14-04-23 06:12:00, Christoph Hellwig wrote:
> On Fri, Apr 14, 2023 at 02:51:48PM +0200, Jan Kara wrote:
> > On Thu 13-04-23 22:59:24, Christoph Hellwig wrote:
> > > Still no fan of the naming and placement here.  This is specific
> > > to the fs/buffer.c infrastructure.
> > 
> > I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and
> > creating the new function there if it makes you happier. But I think
> > function names should be consistent (hence the new function would be named
> > __generic_file_fsync_nolock()). I agree the name is not ideal and would use
> > cleanup (along with transitioning everybody to not take i_rwsem) but I
> > don't want to complicate this series by touching 13+ callsites of
> > generic_file_fsync() and __generic_file_fsync(). That's for a separate
> > series.
> 
> I would not change the existing function.  Just do the right thing for
> the new helper and slowly migrate over without complicating this series.

OK, I can live with that temporary naming inconsistency I guess. So
the function will be __buffer_file_fsync()?

								Honza
Ritesh Harjani (IBM) April 14, 2023, 2:29 p.m. UTC | #5 
Jan Kara <jack@suse.cz> writes:

> On Fri 14-04-23 06:12:00, Christoph Hellwig wrote:
>> On Fri, Apr 14, 2023 at 02:51:48PM +0200, Jan Kara wrote:
>> > On Thu 13-04-23 22:59:24, Christoph Hellwig wrote:
>> > > Still no fan of the naming and placement here.  This is specific
>> > > to the fs/buffer.c infrastructure.
>> >
>> > I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and
>> > creating the new function there if it makes you happier. But I think
>> > function names should be consistent (hence the new function would be named
>> > __generic_file_fsync_nolock()). I agree the name is not ideal and would use
>> > cleanup (along with transitioning everybody to not take i_rwsem) but I
>> > don't want to complicate this series by touching 13+ callsites of
>> > generic_file_fsync() and __generic_file_fsync(). That's for a separate
>> > series.
>>
>> I would not change the existing function.  Just do the right thing for
>> the new helper and slowly migrate over without complicating this series.
>
> OK, I can live with that temporary naming inconsistency I guess. So
> the function will be __buffer_file_fsync()?

This name was suggested before, so if that's ok I will go with this -
"generic_buffer_fsync()". It's definition will lie in fs/buffer.c and
it's declaration in include/linux/buffer_head.h

Is that ok?

-ritesh

>
> 								Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
Jan Kara April 17, 2023, 7:32 a.m. UTC | #6 
On Fri 14-04-23 19:59:42, Ritesh Harjani wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > On Fri 14-04-23 06:12:00, Christoph Hellwig wrote:
> >> On Fri, Apr 14, 2023 at 02:51:48PM +0200, Jan Kara wrote:
> >> > On Thu 13-04-23 22:59:24, Christoph Hellwig wrote:
> >> > > Still no fan of the naming and placement here.  This is specific
> >> > > to the fs/buffer.c infrastructure.
> >> >
> >> > I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and
> >> > creating the new function there if it makes you happier. But I think
> >> > function names should be consistent (hence the new function would be named
> >> > __generic_file_fsync_nolock()). I agree the name is not ideal and would use
> >> > cleanup (along with transitioning everybody to not take i_rwsem) but I
> >> > don't want to complicate this series by touching 13+ callsites of
> >> > generic_file_fsync() and __generic_file_fsync(). That's for a separate
> >> > series.
> >>
> >> I would not change the existing function.  Just do the right thing for
> >> the new helper and slowly migrate over without complicating this series.
> >
> > OK, I can live with that temporary naming inconsistency I guess. So
> > the function will be __buffer_file_fsync()?
> 
> This name was suggested before, so if that's ok I will go with this -
> "generic_buffer_fsync()". It's definition will lie in fs/buffer.c and
> it's declaration in include/linux/buffer_head.h
> 
> Is that ok?

Yes, that is fine by me. And I suppose this variant will also issue the
cache flush, won't it? But then we also need __generic_buffer_fsync()
without issuing the cache flush for ext4 (we need to sync parent before
issuing a cache flush) and FAT.

								Honza
Ritesh Harjani (IBM) April 17, 2023, 10:01 a.m. UTC | #7 
Jan Kara <jack@suse.cz> writes:

> On Fri 14-04-23 19:59:42, Ritesh Harjani wrote:
>> Jan Kara <jack@suse.cz> writes:
>>
>> > On Fri 14-04-23 06:12:00, Christoph Hellwig wrote:
>> >> On Fri, Apr 14, 2023 at 02:51:48PM +0200, Jan Kara wrote:
>> >> > On Thu 13-04-23 22:59:24, Christoph Hellwig wrote:
>> >> > > Still no fan of the naming and placement here.  This is specific
>> >> > > to the fs/buffer.c infrastructure.
>> >> >
>> >> > I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and
>> >> > creating the new function there if it makes you happier. But I think
>> >> > function names should be consistent (hence the new function would be named
>> >> > __generic_file_fsync_nolock()). I agree the name is not ideal and would use
>> >> > cleanup (along with transitioning everybody to not take i_rwsem) but I
>> >> > don't want to complicate this series by touching 13+ callsites of
>> >> > generic_file_fsync() and __generic_file_fsync(). That's for a separate
>> >> > series.
>> >>
>> >> I would not change the existing function.  Just do the right thing for
>> >> the new helper and slowly migrate over without complicating this series.
>> >
>> > OK, I can live with that temporary naming inconsistency I guess. So
>> > the function will be __buffer_file_fsync()?
>>
>> This name was suggested before, so if that's ok I will go with this -
>> "generic_buffer_fsync()". It's definition will lie in fs/buffer.c and
>> it's declaration in include/linux/buffer_head.h
>>
>> Is that ok?
>
> Yes, that is fine by me. And I suppose this variant will also issue the
> cache flush, won't it?

No. We don't issue cache flush (REQ_PREFLUSH) in generic_buffer_fsync(),
neither __generic_file_fsync() does that.

> But then we also need __generic_buffer_fsync()
> without issuing the cache flush for ext4 (we need to sync parent before
> issuing a cache flush) and FAT.

Yes, we do take care of that by -

<simplified logic>
ret = generic_buffer_fsync()
if (!ret)
   ret = ext4_sync_parent(inode)
if (test_opt(inode->i_sb, BARRIER))
   blkdev_issue_flush()

Am I missing anything. I have sent a [v5] with all of the comments
addressed. Could you please take a look and let me know if it looks
good or is there anything required?

[v5]: https://lore.kernel.org/linux-ext4/cover.1681639164.git.ritesh.list@gmail.com/T/#t

-ritesh
diff mbox series

Patch

diff --git a/fs/libfs.c b/fs/libfs.c
index 4eda519c3002..054f2e0ab3cb 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1110,6 +1110,49 @@  struct dentry *generic_fh_to_parent(struct super_block *sb, struct fid *fid,
 }
 EXPORT_SYMBOL_GPL(generic_fh_to_parent);
 
+/**
+ * __generic_file_fsync_nolock - generic fsync implementation for simple
+ * filesystems with no inode lock
+ *
+ * @file:	file to synchronize
+ * @start:	start offset in bytes
+ * @end:	end offset in bytes (inclusive)
+ * @datasync:	only synchronize essential metadata if true
+ *
+ * This is a generic implementation of the fsync method for simple
+ * filesystems which track all non-inode metadata in the buffers list
+ * hanging off the address_space structure.
+ */
+int __generic_file_fsync_nolock(struct file *file, loff_t start, loff_t end,
+				bool datasync)
+{
+	struct inode *inode = file->f_mapping->host;
+	int err;
+	int ret;
+
+	err = file_write_and_wait_range(file, start, end);
+	if (err)
+		return err;
+
+	ret = sync_mapping_buffers(inode->i_mapping);
+	if (!(inode->i_state & I_DIRTY_ALL))
+		goto out;
+	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
+		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(__generic_file_fsync_nolock);
+
 /**
  * __generic_file_fsync - generic fsync implementation for simple filesystems
  *
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c85916e9f7db..9ca3813f43e2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2935,6 +2935,8 @@  extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
 extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
 		const void __user *from, size_t count);
 
+int __generic_file_fsync_nolock(struct file *file, loff_t start, loff_t end,
+				bool datasync);
 extern int __generic_file_fsync(struct file *, loff_t, loff_t, int);
 extern int generic_file_fsync(struct file *, loff_t, loff_t, int);