Message-ID: <YqH7PO7KtoiXkmVH@redhat.com>
Date:   Thu, 9 Jun 2022 09:53:00 -0400
From:   Vivek Goyal <vgoyal@...hat.com>
To:     Bernd Schubert <bschubert@....com>
Cc:     Dharmendra Singh <dharamhans87@...il.com>, miklos@...redi.hu,
        linux-fsdevel@...r.kernel.org, fuse-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, Dharmendra Singh <dsingh@....com>
Subject: Re: [PATCH v4 1/1] Allow non-extending parallel direct writes on the
 same file.

On Wed, Jun 08, 2022 at 12:42:20AM +0200, Bernd Schubert wrote:
> 
> 
> On 6/8/22 00:01, Vivek Goyal wrote:
> > On Tue, Jun 07, 2022 at 11:42:16PM +0200, Bernd Schubert wrote:
> > > 
> > > 
> > > On 6/7/22 23:25, Vivek Goyal wrote:
> > > > On Sun, Jun 05, 2022 at 12:52:00PM +0530, Dharmendra Singh wrote:
> > > > > From: Dharmendra Singh <dsingh@....com>
> > > > > 
> > > > > In general, as of now, in FUSE, direct writes on the same file are
> > > > > serialized over inode lock i.e we hold inode lock for the full duration
> > > > > of the write request. I could not found in fuse code a comment which
> > > > > clearly explains why this exclusive lock is taken for direct writes.
> > > > > 
> > > > > Following might be the reasons for acquiring exclusive lock but not
> > > > > limited to
> > > > > 1) Our guess is some USER space fuse implementations might be relying
> > > > >      on this lock for seralization.
> > > > 
> > > > Hi Dharmendra,
> > > > 
> > > > I will just try to be devil's advocate. So if this is server side
> > > > limitation, then it is possible that fuse client's isize data in
> > > > cache is stale. For example, filesystem is shared between two
> > > > clients.
> > > > 
> > > > - File size is 4G as seen by client A.
> > > > - Client B truncates the file to 2G.
> > > > - Two processes in client A, try to do parallel direct writes and will
> > > >     be able to proceed and server will get two parallel writes both
> > > >     extending file size.
> > > > 
> > > > I can see that this can happen with virtiofs with cache=auto policy.
> > > > 
> > > > IOW, if this is a fuse server side limitation, then how do you ensure
> > > > that fuse kernel's i_size definition is not stale.
> > > 
> > > Hi Vivek,
> > > 
> > > I'm sorry, to be sure, can you explain where exactly a client is located for
> > > you? For us these are multiple daemons linked to libufse - which you seem to
> > > call 'server' Typically these clients are on different machines. And servers
> > > are for us on the other side of the network - like an NFS server.
> > 
> > Hi Bernd,
> > 
> > Agreed, terminology is little confusing. I am calling "fuse kernel" as
> > client and fuse daemon (user space) as server. This server in turn might
> > be the client to another network filesystem and real files might be
> > served by that server on network.
> > 
> > So for simple virtiofs case, There can be two fuse daemons (virtiofsd
> > instances) sharing same directory (either on local filesystem or on
> > a network filesystem).
> 
> So the combination of fuse-kernel + fuse-daemon == vfs mount.

This is fine for regular fuse file systems. For virtiofs fuse-kernel is
running in a VM and fuse-daemon is running outside the VM on host.
> 
> > 
> > > 
> > > So now while I'm not sure what you mean with 'client', I'm wondering about
> > > two generic questions
> > > 
> > > a) I need to double check, but we were under the assumption the code in
> > > question is a direct-io code path. I assume cache=auto would use the page
> > > cache and should not be effected?
> > 
> > By default cache=auto use page cache but if application initiates a
> > direct I/O, it should use direct I/O path.
> 
> Ok, so we are on the same page regarding direct-io.
> 
> > 
> > > 
> > > b) How would the current lock help for distributed clients? Or multiple fuse
> > > daemons (what you seem to call server) per local machine?
> > 
> > I thought that current lock is trying to protect fuse kernel side and
> > assumed fuse server (daemon linked to libfuse) can handle multiple
> > parallel writes. Atleast that's how I thought about the things. I might
> > be wrong. I am not sure.
> > 
> > > 
> > > For a single vfs mount point served by fuse, truncate should take the
> > > exclusive lock and parallel writes the shared lock - I don't see a problem
> > > here either.
> > 
> > Agreed that this does not seem like a problem from fuse kernel side. I was
> > just questioning that where parallel direct writes become a problem. And
> > answer I heard was that it probably is fuse server (daemon linked with
> > libfuse) which is expecting the locking. And if that's the case, this
> > patch is not fool proof. It is possible that file got truncated from
> > a different client (from a different fuse daemon linked with libfuse).
> > 
> > So say A is first fuse daemon and B is another fuse daemon. Both are
> > clients to some network file system as NFS.
> > 
> > - Fuse kernel for A, sees file size as 4G.
> > - fuse daemon B truncates the file to size 2G.
> > - Fuse kernel for A, has stale cache, and can send two parallel writes
> >    say at 3G and 3.5G offset.
> 
> I guess you mean inode cache, not data cache, as this is direct-io.

Yes inode cache and cached ->i_size might be an issue. These patches
used cached ->i_size to determine if parallel direct I/O should be
allowed or not.


> But now
> why would we need to worry about any cache here, if this is direct-io - the
> application writes without going into any cache and at the same time a
> truncate happens? The current kernel side lock would not help here, but a
> distrubuted lock is needed to handle this correctly?
> 
> int fd = open(path, O_WRONLY | O_DIRECT);
> 
> clientA: pwrite(fd, buf, 100G, 0) -> takes a long time
> clientB: ftruncate(fd, 0)
> 
> I guess on a local file system that will result in a zero size file. On
> different fuse mounts (without a DLM) or NFS, undefined behavior.
> 
> 
> > - Fuser daemon A might not like it.(Assuming this is fuse daemon/user
> >    space side limitation).
> 
> I think there are two cases for the fuser daemons:
> 
> a) does not have a distributed lock - just needs to handle the writes, the
> local kernel lock does not protect against distributed races.

Exactly. This is the point I am trying to raise. "Local kernel lock does
not protect against distributed races".

So in this case local kernel has ->i_size cached and this might be an
old value and checking i_size does not guarantee that fuse daemon
will not get parallel extending writes.

> I guess most
> of these file systems can enable parallel writes, unless the kernel lock is
> used to handle userspace thread synchronization.

Right. If user space is relying on kernel lock for thread synchronization,
it can not enable parallel writes.

But if it is not relying on this, it should be able to enable parallel
writes. Just keep in mind that ->i_size check is not sufficient to
guarantee that you will not get "two extnding parallel writes". If
another client on a different machine truncated the file, it is
possible this client has old cached ->i_size and it will can
get multiple file extending parallel writes.

So if fuse daemon enables parallel extending writes, it should be
prepared to deal with multiple extending parallel writes.

And if this is correct assumption, I am wondering why to even try
to do ->i_size check and try to avoid parallel extending writes
in fuse kernel. May be there is something I am not aware of. And
that's why I am just raising questions.

> 
> b) has a distributed lock - needs a callback to fuse kernel to inform the
> kernel to invalidate all data.
> 
> At DDN we have both of them, a) is in production, the successor b) is being
> worked on. We might come back with more patches for more callbacks for the
> DLM - I'm not sure yet.
> 
> 
> > 
> > I hope I am able to explain my concern. I am not saying that this patch
> > is not good. All I am saying that fuse daemon (user space) can not rely
> > on that it will never get two parallel direct writes which can be beyond
> > the file size. If fuse kernel cache is stale, it can happen. Just trying
> > to set the expectations right.
> 
> 
> I don't see an issue yet. Regarding virtiofs, does it have a distributed
> lock manager (DLM)? I guess not?

Nope. virtiofs does not have any DLM.

Vivek
> 
> 
> Thanks,
> Bernd
> 

Powered by blists - more mailing lists