Re: 9pfs: scope of rename_lock?

From: Christian Schoenebeck <qemu_oss@crudebyte.com>
To: qemu-devel@nongnu.org
Cc: Greg Kurz <groug@kaod.org>,
	"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Subject: Re: 9pfs: scope of rename_lock?
Date: Tue, 25 May 2021 13:41:22 +0200	[thread overview]
Message-ID: <3482348.c50Dd4UUo8@silver> (raw)
In-Reply-To: <20210521135947.6ea005e5@bahia.lan>

On Freitag, 21. Mai 2021 13:59:47 CEST Greg Kurz wrote:
> On Sun, 16 May 2021 19:06:44 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Hi Greg,
> > 
> > while reviewing the 9p code base for further optimizations, I stumbled
> > over
> > the 'rename_lock' introduced by 02cb7f3a2 and wondered about what exactly
> > it shall protect?
> > 
> > As far as I understand it, the original intention at introduction
> > (aforementioned 02cb7f3a2) was to protect
> > 
> > 	1. fidp->path variable
> > 	
> > 	and
> > 	
> > 	2.  *ANY* filesystem path from being renamed during the *entire* 
duration
> > 	
> > 	    of some concurrent 9p operation.
> > 
> > So because of (2.) it was introduced as a global lock. But (2.) is a dead
> > end approach anyway, isn't it?
> 
> I agree that this looks terrible.
> 
> > Therefore my question: rename_lock is currently a global lock. Wouldn't it
> > make more sense to transform it from a global lock from struct V9fsState
> > ->
> > struct V9fsFidState and just let it protect that fidp->path variable
> > locally there?
> 
> Nothing comes to the top of my mind right now... Maybe Aneesh (Cc'd) can
> shed some light on:
> 
> commit 02cb7f3a256517cbf3136caff2863fbafc57b540
> Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Date:   Tue May 24 15:10:56 2011 +0530
> 
>     hw/9pfs: Use read-write lock for protecting fid path.
> 
>     On rename we take the write lock and this ensure path
>     doesn't change as we operate on them.
> 
>     Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> 
> 
> Why are we serializing all operations on all fid paths with a single
> global lock ?

I started to work on a patch set on this.

I try to get rid of that rename_lock entirely by letting the worker threads 
only access temporary copies e.g. of the fid path instead of allowing the 
worker threads to access main thread owned structures directly like it is the 
case ATM.

I also noticed that the rename_lock scheme is quite inconsistent right now 
anyway. E.g. some of the fs v9fs_co_*() functions accessing main thread owned 
structures don't take the lock at all. Some for good reasons, some not.

So this week I will give the described approach above a test spin and then we 
will see how this impacts performance in practice before actually posting any 
patch set here.

Best regards,
Christian Schoenebeck