Received-SPF: pass (google.com: domain of linux-nfs+bounces-3370-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 24 May 2024 15:17:55 +0200
From: Jan Kara <jack@suse.cz>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "jack@suse.cz" <jack@suse.cz>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"anna@kernel.org" <anna@kernel.org>,
	"jlayton@kernel.org" <jlayton@kernel.org>,
	"chuck.lever@oracle.com" <chuck.lever@oracle.com>
Subject: Re: Bad NFS performance for fsync(2)
Message-ID: <20240524131755.xtgt7zgz4ibdfdu7@quack3>
References: <20240523165436.g5xgo7aht7dtmvfb@quack3>
 <271717b8f2a6b2121dbb529ed3a21a69467b0fa5.camel@hammerspace.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <271717b8f2a6b2121dbb529ed3a21a69467b0fa5.camel@hammerspace.com>

Hello Trond!

On Thu 23-05-24 18:00:01, Trond Myklebust wrote:
> On Thu, 2024-05-23 at 18:54 +0200, Jan Kara wrote:
> > Hello!
> > 
> > I've been debugging NFS performance regression with recent kernels.
> > It
> > seems to be at least partially related to the following behavior of
> > NFS
> > (which is there for a long time AFAICT). Suppose the following
> > workload:
> > 
> > fio --direct=0 --ioengine=sync --thread --directory=/test --
> > invalidate=1 \
> > ? --group_reporting=1 --runtime=100 --fallocate=posix --ramp_time=10
> > \
> > ? --name=RandomWrites-async --new_group --rw=randwrite --size=32000m
> > \
> > ? --numjobs=4 --bs=4k --fsync_on_close=1 --end_fsync=1 \
> > ? --filename_format='FioWorkloads.$jobnum'
> > 
> > So we do 4k buffered random writes from 4 threads into 4 different
> > files.
> > Now the interesting behavior comes on the final fsync(2). What I
> > observe is
> > that the NFS server getting a stream of 4-8k writes which have
> > 'stable'
> > flag set. What the server does for each such write is that performs
> > the
> > write and calls fsync(2). Since by the time fio calls fsync(2) on the
> > NFS
> > client there is like 6-8 GB worth of dirty pages to write and the
> > server
> > effectively ends up writing each individual 4k page as O_SYNC write,
> > the
> > throughput is not great...
> > 
> > The reason why the client sets 'stable' flag for each page write
> > seems to
> > be because nfs_writepages() issues writes with FLUSH_COND_STABLE for
> > WB_SYNC_ALL writeback and nfs_pgio_rpcsetup() has this logic:
> > 
> > ??????? switch (how & (FLUSH_STABLE | FLUSH_COND_STABLE)) {
> > ??????? case 0:
> > ??????????????? break;
> > ??????? case FLUSH_COND_STABLE:
> > ??????????????? if (nfs_reqs_to_commit(cinfo))
> > ??????????????????????? break;
> > ??????????????? fallthrough;
> > ??????? default:
> > ??????????????? hdr->args.stable = NFS_FILE_SYNC;
> > ??????? }
> > 
> > but since this is final fsync(2), there are no more requests to
> > commit so
> > we set NFS_FILE_SYNC flag.
> > 
> > Now I'd think the client is stupid in submitting so many
> > NFS_FILE_SYNC
> > writes instead of submitting all as async and then issuing commit
> > (i.e.,
> > the switch above in nfs_pgio_rpcsetup() could gain something like:
> > 
> > 		if (count > <small_magic_number>)
> > 			break;
> > 
> > But I'm not 100% sure this is a correct thing to do since I'm not
> > 100% sure
> > about the FLUSH_COND_STABLE requirements. On the other hand it could
> > be
> > also argued that the NFS server could be more clever and batch the
> > fsync(2)s for many sync writes to the same file. But there the
> > heuristic is
> > less clear.
> > 
> > So what do people think?
> 
> We can probably remove that case FLUSH_COND_STABLE in
> nfs_pgio_rpcsetup() altogether, since we have the following just before
> the call to nfs_pgio_rpcsetup()
> 
>         if ((desc->pg_ioflags & FLUSH_COND_STABLE) &&
>             (desc->pg_moreio || nfs_reqs_to_commit(&cinfo)))
>                 desc->pg_ioflags &= ~FLUSH_COND_STABLE;

Yeah, I was somewhat wondering about that as well. But my point was that
the client should apparently be dropping FLUSH_COND_STABLE in more cases
because currently fsync(2) submits several GB worth of dirty pages, each
page with NFS_FILE_SYNC set. Which is suboptimal... I'll try to understand
better why non of the above conditions is met.

> The above is telling you that if we're flushing because we cannot
> coalesce any more in __nfs_pageio_add_request(), then we do an unstable
> write. Ditto if there are already unstable requests waiting for a
> COMMIT.

Thanks for explanation! It helps me better understand the NFS page state
machinery :)

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