[PATCH 0/2] virtiofs: miscellaneous fixes

[PATCH 0/2] virtiofs: miscellaneous fixes

[Jeffle Xu]

Some fixes or optimization for virtiofs, which are authored by Liu Bo.

Liu Bo (2):
  virtio-fs: disable atomic_o_trunc if no_open is enabled
  virtiofs: reduce lock contention on fpq->lock

 fs/fuse/file.c      | 11 +++++++++--
 fs/fuse/virtio_fs.c |  3 ---
 2 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.27.0

[PATCH 1/2] fuse: disable atomic_o_trunc if no_open is enabled

[Jeffle Xu]

From: Liu Bo <bo.liu@linux.alibaba.com>

When 'no_open' is used by virtiofsd, guest kernel won't send OPEN request
any more.  However, with atomic_o_trunc, SETATTR request is also omitted in
OPEN(O_TRUNC) so that the backend file is not truncated.  With a following
GETATTR, inode size on guest side is updated to be same with that on host
side, the end result is that O_TRUNC semantic is broken.

This disables atomic_o_trunc as well if with no_open.

Reviewed-by: Peng Tao <tao.peng@linux.alibaba.com>
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/file.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index b494ff08f08c..1231128f8dd6 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -151,10 +151,16 @@ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
 			fuse_file_free(ff);
 			return ERR_PTR(err);
 		} else {
-			if (isdir)
+			if (isdir) {
 				fc->no_opendir = 1;
-			else
+			} else {
 				fc->no_open = 1;
+				/*
+				 * In case of no_open, disable atomic_o_trunc as
+				 * well.
+				 */
+				fc->atomic_o_trunc = 0;
+			}
 		}
 	}
 
-- 
2.27.0

Re: [PATCH 1/2] fuse: disable atomic_o_trunc if no_open is enabled

[Miklos Szeredi]

On Thu, 12 Aug 2021 at 07:46, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>
> From: Liu Bo <bo.liu@linux.alibaba.com>
>
> When 'no_open' is used by virtiofsd, guest kernel won't send OPEN request
> any more.  However, with atomic_o_trunc, SETATTR request is also omitted in
> OPEN(O_TRUNC) so that the backend file is not truncated.  With a following
> GETATTR, inode size on guest side is updated to be same with that on host
> side, the end result is that O_TRUNC semantic is broken.
>
> This disables atomic_o_trunc as well if with no_open.

I don't quite get it why one would want to enable atomic_o_trunc with
no_open in the first place?

Thanks,
Miklos

Re: [PATCH 1/2] fuse: disable atomic_o_trunc if no_open is enabled

[JeffleXu]

On 9/7/21 4:34 PM, Miklos Szeredi wrote:
> On Thu, 12 Aug 2021 at 07:46, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>>
>> From: Liu Bo <bo.liu@linux.alibaba.com>
>>
>> When 'no_open' is used by virtiofsd, guest kernel won't send OPEN request
>> any more.  However, with atomic_o_trunc, SETATTR request is also omitted in
>> OPEN(O_TRUNC) so that the backend file is not truncated.  With a following
>> GETATTR, inode size on guest side is updated to be same with that on host
>> side, the end result is that O_TRUNC semantic is broken.
>>
>> This disables atomic_o_trunc as well if with no_open.
> 
> I don't quite get it why one would want to enable atomic_o_trunc with
> no_open in the first place?

Oops..We didn't realize that it could also be worked around by fuse
daemon side. Please ignore this.

-- 
Thanks,
Jeffle

[PATCH 2/2] virtiofs: reduce lock contention on fpq->lock

[Jeffle Xu]

From: Liu Bo <bo.liu@linux.alibaba.com>

Since %req has been removed from fpq->processing_list, no one except
request_wait_answer() is looking at this %req and request_wait_answer()
waits only on FINISH flag, it's OK to remove fpq->lock after %req is
dropped from the list.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/virtio_fs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 0050132e2787..7dbf5502c57e 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -557,7 +557,6 @@ static void copy_args_from_argbuf(struct fuse_args *args, struct fuse_req *req)
 static void virtio_fs_request_complete(struct fuse_req *req,
 				       struct virtio_fs_vq *fsvq)
 {
-	struct fuse_pqueue *fpq = &fsvq->fud->pq;
 	struct fuse_args *args;
 	struct fuse_args_pages *ap;
 	unsigned int len, i, thislen;
@@ -586,9 +585,7 @@ static void virtio_fs_request_complete(struct fuse_req *req,
 		}
 	}
 
-	spin_lock(&fpq->lock);
 	clear_bit(FR_SENT, &req->flags);
-	spin_unlock(&fpq->lock);
 
 	fuse_request_end(req);
 	spin_lock(&fsvq->lock);
-- 
2.27.0

Re: [PATCH 2/2] virtiofs: reduce lock contention on fpq->lock

[Miklos Szeredi]

On Thu, 12 Aug 2021 at 07:46, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>
> From: Liu Bo <bo.liu@linux.alibaba.com>
>
> Since %req has been removed from fpq->processing_list, no one except
> request_wait_answer() is looking at this %req and request_wait_answer()
> waits only on FINISH flag, it's OK to remove fpq->lock after %req is
> dropped from the list.

I'll accept a patch to remove FR_SENT completely from virtiofs.

This flag is used for queuing interrupts but interrupts are not yet
implemented in virtiofs.    When blocking lock support is added the
interrupt handling needs to be properly designed.

Thanks,
Miklos

Re: [PATCH 2/2] virtiofs: reduce lock contention on fpq->lock

[Vivek Goyal]

On Tue, Sep 07, 2021 at 10:57:07AM +0200, Miklos Szeredi wrote:
> On Thu, 12 Aug 2021 at 07:46, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
> >
> > From: Liu Bo <bo.liu@linux.alibaba.com>
> >
> > Since %req has been removed from fpq->processing_list, no one except
> > request_wait_answer() is looking at this %req and request_wait_answer()
> > waits only on FINISH flag, it's OK to remove fpq->lock after %req is
> > dropped from the list.
> 
> I'll accept a patch to remove FR_SENT completely from virtiofs.
> 

Recently I was also looking at FR_SENT flag and was wondering if it
is atomic bit flag, then why do we need to take spin lock around it.
Probably we need just some barrier if code needs it but not necessarily
any lock.

But I agree that FR_SENT seems not usable from virtiofs point of view
as we don't have support for interrupt request.

> This flag is used for queuing interrupts but interrupts are not yet
> implemented in virtiofs.    When blocking lock support is added the
> interrupt handling needs to be properly designed.

Hmm.., I did not think about this. I was getting ready to post patches
for blocking posix locks but it does not have any support for interrupting
the locking request (either blocked or queued).

Is implementing interrupt support a requirement for getting blocking
posix lock patches in?

Thanks
Vivek

Re: [PATCH 0/2] virtiofs: miscellaneous fixes

[JeffleXu]

ping ...

On 8/12/21 1:46 PM, Jeffle Xu wrote:
> Some fixes or optimization for virtiofs, which are authored by Liu Bo.
> 
> Liu Bo (2):
>   virtio-fs: disable atomic_o_trunc if no_open is enabled
>   virtiofs: reduce lock contention on fpq->lock
> 
>  fs/fuse/file.c      | 11 +++++++++--
>  fs/fuse/virtio_fs.c |  3 ---
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 

-- 
Thanks,
Jeffle