Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 18:45:50 +0200
From:   Jan Kara <jack@suse.cz>
To:     Ritesh Harjani <ritesh.list@gmail.com>
Cc:     Jan Kara <jack@suse.cz>, linux-fsdevel@vger.kernel.org,
        linux-ext4@vger.kernel.org, Christoph Hellwig <hch@infradead.org>,
        "Darrick J . Wong" <djwong@kernel.org>,
        Ojaswin Mujoo <ojaswin@linux.ibm.com>,
        Disha Goel <disgoel@linux.ibm.com>,
        Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCHv5 2/9] fs/buffer.c: Add generic_buffer_fsync
 implementation
Message-ID: <20230417164550.yw6p4ddruutxqqax@quack3>
References: <20230417110149.mhrksh4owqkfw5pa@quack3>
 <87o7nmivqm.fsf@doe.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87o7nmivqm.fsf@doe.com>
Precedence: bulk

On Mon 17-04-23 17:08:57, Ritesh Harjani wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > On Sun 16-04-23 15:38:37, Ritesh Harjani (IBM) wrote:
> >> 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
> >> generic_buffer_fsync() implementation in buffer.c with no
> >> inode_lock/unlock() 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
> >> generic_buffer_fsync() which does not take any inode_lock() as the
> >> default path.
> >>
> >> Tested-by: Disha Goel <disgoel@linux.ibm.com>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> >
> > There is a problem with generic_buffer_fsync() that it does not call
> > blkdev_issue_flush() so the caller is responsible for doing that. That's
> > necessary for ext2 & ext4 so fine for now. But historically this was the
> > case with generic_file_fsync() as well and that led to many filesystem
> > forgetting to flush caches from fsync(2).
> 
> Ok, thanks for the details.
> 
> > What is our transition plan for
> > these filesystems that currently do the cache flush from
> > generic_file_fsync()? Do we want to eventually keep generic_file_fsync()
> > doing the cache flush and call generic_buffer_fsync() instead of
> > __generic_buffer_fsync() from it?
> 
> Frankly speaking, I was thinking we will come back to this question
> maybe when we start working on those changes. At this point in time
> I only looked at it from ext2 DIO changes perspective.

Yes, we can return to this later. The only thing I wanted to kind of make
sure is we don't have to rename the function again when adding support for
other filesystems (although even that would not be a big issue given there
are two callers).

> But since you asked, here is what I think we could do -
> 
> Rename generic_file_fsync => generic_buffers_sync() to fs/buffers.c
> Then
> generic_buffers_sync() {
>     ret = generic_buffers_fsync()
>     if (!ret)
>        blkdev_issue_flush()
> }
> 
> generic_buffers_fsync() is same as in this patch which does not have the
> cache flush operation.
> (will rename from generic_buffer_fsync() to generic_buffers_fsync())
> 
> Note: The naming is kept such that-
> - sync means it will do fsync followed by cache flush.
> - fsync means it will only do the file fsync

Hum, I think the difference sync vs fsync is too subtle and non-obvious.
I can see sensible pairs like:

	__generic_buffers_fsync() - "__" indicates you should know what you
				are doing when calling this
	generic_buffers_fsync()

or

	generic_buffers_fsync()
	generic_file_fsync() - difficult at this point as there's name
			       clash

or

	generic_buffers_fsync_noflush()
	generic_buffers_fsync() - obvious what the default "safe" choice
				  is.

or something like that.

> As I understand - we would eventually like to kill the
> inode_lock() variants of generic_file_fsync() and __generic_file_fsync()
> after auditing other filesystem code, right?

Yes.

> Then for now what we need is generic_buffers_sync() function which does
> not take an inode_lock() and also does cache flush which is required for ext2.
> And generic_buffers_fsync() which does not do any cache flush operations
> required by filesystem like ext4.
> 
> Does that sound good to you? Is the naming also proper?

I agree with the plan, just the naming is hard :)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR