Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 8/29/23 15:08, Bernd Schubert wrote:
> 
> 
> On 8/28/23 17:05, Miklos Szeredi wrote:
>> On Mon, 28 Aug 2023 at 16:48, Bernd Schubert 
>> <bernd.schubert@fastmail.fm> wrote:
>>>
>>> On 8/28/23 13:59, Miklos Szeredi wrote:
>>>> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>>>>> -               if (!is_sync_kiocb(iocb) && iocb->ki_flags & 
>>>>> IOCB_DIRECT) {
>>>>> -                       res = fuse_direct_IO(iocb, from);
>>>>> -               } else {
>>>>> -                       res = fuse_direct_io(&io, from, &iocb->ki_pos,
>>>>> -                                            FUSE_DIO_WRITE);
>>>>> -                       fuse_write_update_attr(inode, iocb->ki_pos, 
>>>>> res);
>>>>
>>>> While I think this is correct, I'd really like if the code to be
>>>> replaced and the replacement are at least somewhat comparable.
>>>
>>> Sorry, I have a hard to time to understand "I'd really like if the code
>>> to be replaced".
>>
>> What I meant is that generic_file_direct_write() is not an obvious
>> replacement for the  above lines of code.
>>
>> The reason is that fuse_direct_IO() is handling the sync and async
>> cases in one function, while the above splits handling it based on
>> IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb).  If it's okay
>> to lose IOCB_DIRECT then what's the explanation for the above
>> condition?  It could be historic garbage, but we still need to
>> understand what is exactly happening.
> 
> While checking all code path again, I found an additional difference, 
> which I had missed before. FOPEN_DIRECT_IO will now act on 
> ff->fm->fc->async_dio when is is_sync_kiocb(iocb) is set.
> 
> Do you think that is a problem? If so, I could fix it in fuse_direct_IO.

What I mean is something like this

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 85f2f9d3813e..3b383dc8a944 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1635,8 +1635,10 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
  	if (FUSE_IS_DAX(inode))
  		return fuse_dax_write_iter(iocb, from);
  
-	if (ff->open_flags & FOPEN_DIRECT_IO)
+	if (ff->open_flags & FOPEN_DIRECT_IO) {
  		iocb->ki_flags |= IOCB_DIRECT;
+		ff->iocb_direct = 1;
+	}
  
  	return fuse_cache_write_iter(iocb, from);
  }
@@ -2905,6 +2907,15 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
  	io->iocb = iocb;
  	io->blocking = is_sync_kiocb(iocb);
  
+	/* FOPEN_DIRECT_IO historically does not use async for blocking O_DIRECT */
+	if (ff->open_flags & FOPEN_DIRECT_IO) {
+		if (!is_sync_kiocb(iocb) && ff->iocb_direct) {
+			/* no change */
+		} else {
+			io->async = 0;
+		}
+	}
+
  	/* optimization for short read */
  	if (io->async && !io->write && offset + count > i_size) {
  		iov_iter_truncate(iter, fuse_round_up(ff->fm->fc, i_size - offset));
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index a56e83b7d29a..d77046875ad5 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -231,6 +231,9 @@ struct fuse_file {
  
  	/** Has flock been performed on this file? */
  	bool flock:1;
+
+	/** Has the file been opened with O_DIRECT? */
+	bool iocb_direct:1;
  };
  
  /** One input argument of a request */


Thanks,
Bernd