gfs2.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: NeilBrown <neilb@suse.de>
Cc: Christian Brauner <brauner@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Eric Van Hensbergen <ericvh@kernel.org>,
	 Latchesar Ionkov <lucho@ionkov.net>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Christian Schoenebeck <linux_oss@crudebyte.com>,
	David Howells <dhowells@redhat.com>,
	Marc Dionne <marc.dionne@auristor.com>,
	Xiubo Li <xiubli@redhat.com>, Ilya Dryomov <idryomov@gmail.com>,
	Alexander Aring <aahringo@redhat.com>,
	David Teigland <teigland@redhat.com>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Andreas Gruenbacher <agruenba@redhat.com>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	 Anna Schumaker <anna@kernel.org>,
	Chuck Lever <chuck.lever@oracle.com>,
	Olga Kornievskaia <kolga@netapp.com>,
	Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
	 Jan Kara <jack@suse.cz>, Mark Fasheh <mark@fasheh.com>,
	Joel Becker <jlbec@evilplan.org>,
	Joseph Qi <joseph.qi@linux.alibaba.com>,
	Steve French <sfrench@samba.org>,
	Paulo Alcantara <pc@manguebit.com>,
	Ronnie Sahlberg <lsahlber@redhat.com>,
	Shyam Prasad N <sprasad@microsoft.com>,
	Namjae Jeon <linkinjeon@kernel.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	 linux-kernel@vger.kernel.org, v9fs@lists.linux.dev,
	 linux-afs@lists.infradead.org, ceph-devel@vger.kernel.org,
	gfs2@lists.linux.dev,  linux-fsdevel@vger.kernel.org,
	linux-nfs@vger.kernel.org,  ocfs2-devel@lists.linux.dev,
	linux-cifs@vger.kernel.org,  samba-technical@lists.samba.org,
	linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH 12/20] filelock: make __locks_delete_block and __locks_wake_up_blocks take file_lock_core
Date: Wed, 17 Jan 2024 07:40:39 -0500	[thread overview]
Message-ID: <6d7f8219f761bec5661906de80cbbb9b75db900d.camel@kernel.org> (raw)
In-Reply-To: <170544378717.23031.5597414508293858294@noble.neil.brown.name>

On Wed, 2024-01-17 at 09:23 +1100, NeilBrown wrote:
> On Wed, 17 Jan 2024, Jeff Layton wrote:
> > Convert __locks_delete_block and __locks_wake_up_blocks to take a struct
> > file_lock_core pointer. Note that to accomodate this, we need to add a
> > new file_lock() wrapper to go from file_lock_core to file_lock.
> 
> Actually we don't need it.... see below.
> 
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/locks.c | 43 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index eddf4d767d5d..6b8e8820dec9 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -92,6 +92,11 @@ static inline bool IS_LEASE(struct file_lock_core *flc)
> >  
> >  #define IS_REMOTELCK(fl)	(fl->fl_core.fl_pid <= 0)
> >  
> > +struct file_lock *file_lock(struct file_lock_core *flc)
> > +{
> > +	return container_of(flc, struct file_lock, fl_core);
> > +}
> > +
> >  static bool lease_breaking(struct file_lock *fl)
> >  {
> >  	return fl->fl_core.fl_flags & (FL_UNLOCK_PENDING | FL_DOWNGRADE_PENDING);
> > @@ -677,31 +682,35 @@ static void locks_delete_global_blocked(struct file_lock_core *waiter)
> >   *
> >   * Must be called with blocked_lock_lock held.
> >   */
> > -static void __locks_delete_block(struct file_lock *waiter)
> > +static void __locks_delete_block(struct file_lock_core *waiter)
> >  {
> > -	locks_delete_global_blocked(&waiter->fl_core);
> > -	list_del_init(&waiter->fl_core.fl_blocked_member);
> > +	locks_delete_global_blocked(waiter);
> > +	list_del_init(&waiter->fl_blocked_member);
> >  }
> >  
> > -static void __locks_wake_up_blocks(struct file_lock *blocker)
> > +static void __locks_wake_up_blocks(struct file_lock_core *blocker)
> >  {
> > -	while (!list_empty(&blocker->fl_core.fl_blocked_requests)) {
> > -		struct file_lock *waiter;
> > +	while (!list_empty(&blocker->fl_blocked_requests)) {
> > +		struct file_lock_core *waiter;
> > +		struct file_lock *fl;
> > +
> > +		waiter = list_first_entry(&blocker->fl_blocked_requests,
> > +					  struct file_lock_core, fl_blocked_member);
> >  
> > -		waiter = list_first_entry(&blocker->fl_core.fl_blocked_requests,
> > -					  struct file_lock, fl_core.fl_blocked_member);
> 
> > +		fl = file_lock(waiter);
> 
> 		fl = list_first_entry(&blocker->fl_core.fl_blocked_requests,
> 				      struct file_lock, fl_core.fl_blocked_member);
> 
>                 waiter = &fl->fl_core;
> 
> achieves the same result without needing file_lock().
> 
> If you really want to add file_lock() then do so, but you need a better
> justification :-)
> 
> 

Except that we actually do need to pass the file_lock pointer to
lm_notify in the block below. There are also some other places that come
up later that will need file_lock() (and file_lease(), for that matter).
I probably could get away without the container_of wrappers, but then
we'd need separate functions for leases for some of the fs/locks.c code.

To be clear, this is not the "cleanest" split ever. If we were
reimplementing leases from scratch today, I'd probably not piggyback on
so much of the file locking code. My main goal here is to get the
initial struct split done. Once that's finished, we may be able to do a
more clean separation later.

> 
> 
> >  		__locks_delete_block(waiter);
> > -		if (waiter->fl_lmops && waiter->fl_lmops->lm_notify)
> > -			waiter->fl_lmops->lm_notify(waiter);
> > +		if ((IS_POSIX(waiter) || IS_FLOCK(waiter)) &&
> > +		    fl->fl_lmops && fl->fl_lmops->lm_notify)
> > +			fl->fl_lmops->lm_notify(fl);


> >  		else
> > -			wake_up(&waiter->fl_core.fl_wait);
> > +			wake_up(&waiter->fl_wait);
> >  
> >  		/*
> >  		 * The setting of fl_blocker to NULL marks the "done"
> >  		 * point in deleting a block. Paired with acquire at the top
> >  		 * of locks_delete_block().
> >  		 */
> > -		smp_store_release(&waiter->fl_core.fl_blocker, NULL);
> > +		smp_store_release(&waiter->fl_blocker, NULL);
> >  	}
> >  }
> >  
> > @@ -743,8 +752,8 @@ int locks_delete_block(struct file_lock *waiter)
> >  	spin_lock(&blocked_lock_lock);
> >  	if (waiter->fl_core.fl_blocker)
> >  		status = 0;
> > -	__locks_wake_up_blocks(waiter);
> > -	__locks_delete_block(waiter);
> > +	__locks_wake_up_blocks(&waiter->fl_core);
> > +	__locks_delete_block(&waiter->fl_core);
> >  
> >  	/*
> >  	 * The setting of fl_blocker to NULL marks the "done" point in deleting
> > @@ -799,7 +808,7 @@ static void __locks_insert_block(struct file_lock *blocker,
> >  	 * waiter, but might not conflict with blocker, or the requests
> >  	 * and lock which block it.  So they all need to be woken.
> >  	 */
> > -	__locks_wake_up_blocks(waiter);
> > +	__locks_wake_up_blocks(&waiter->fl_core);
> >  }
> >  
> >  /* Must be called with flc_lock held. */
> > @@ -831,7 +840,7 @@ static void locks_wake_up_blocks(struct file_lock *blocker)
> >  		return;
> >  
> >  	spin_lock(&blocked_lock_lock);
> > -	__locks_wake_up_blocks(blocker);
> > +	__locks_wake_up_blocks(&blocker->fl_core);
> >  	spin_unlock(&blocked_lock_lock);
> >  }
> >  
> > @@ -1186,7 +1195,7 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
> >  			 * Ensure that we don't find any locks blocked on this
> >  			 * request during deadlock detection.
> >  			 */
> > -			__locks_wake_up_blocks(request);
> > +			__locks_wake_up_blocks(&request->fl_core);
> >  			if (likely(!posix_locks_deadlock(request, fl))) {
> >  				error = FILE_LOCK_DEFERRED;
> >  				__locks_insert_block(fl, request,
> > 
> > -- 
> > 2.43.0
> > 
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

  reply	other threads:[~2024-01-17 12:40 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-16 19:45 [PATCH 00/20] filelock: split struct file_lock into file_lock and file_lease structs Jeff Layton
2024-01-16 19:45 ` [PATCH 01/20] filelock: split common fields into struct file_lock_core Jeff Layton
2024-01-16 22:07   ` NeilBrown
2024-01-17 12:46     ` Jeff Layton
2024-01-16 19:45 ` [PATCH 02/20] filelock: add coccinelle scripts to move fields to " Jeff Layton
2024-01-16 19:45 ` [PATCH 03/20] filelock: the results of the coccinelle conversion Jeff Layton
2024-01-17 15:10   ` Chuck Lever
2024-01-16 19:46 ` [PATCH 04/20] filelock: fixups after the coccinelle changes Jeff Layton
2024-01-17 15:13   ` Chuck Lever
2024-01-16 19:46 ` [PATCH 05/20] filelock: convert some internal functions to use file_lock_core instead Jeff Layton
2024-01-16 19:46 ` [PATCH 06/20] filelock: convert more internal functions to use file_lock_core Jeff Layton
2024-01-16 19:46 ` [PATCH 07/20] filelock: make posix_same_owner take file_lock_core pointers Jeff Layton
2024-01-16 19:46 ` [PATCH 08/20] filelock: convert posix_owner_key to take file_lock_core arg Jeff Layton
2024-01-16 19:46 ` [PATCH 09/20] filelock: make locks_{insert,delete}_global_locks " Jeff Layton
2024-01-16 19:46 ` [PATCH 10/20] filelock: convert locks_{insert,delete}_global_blocked Jeff Layton
2024-01-16 19:46 ` [PATCH 11/20] filelock: convert the IS_* macros to take file_lock_core Jeff Layton
2024-01-16 22:16   ` NeilBrown
2024-01-17 12:32     ` Jeff Layton
2024-01-16 19:46 ` [PATCH 12/20] filelock: make __locks_delete_block and __locks_wake_up_blocks " Jeff Layton
2024-01-16 22:23   ` NeilBrown
2024-01-17 12:40     ` Jeff Layton [this message]
2024-01-16 19:46 ` [PATCH 13/20] filelock: convert __locks_insert_block, conflict and deadlock checks to use file_lock_core Jeff Layton
2024-01-16 22:32   ` NeilBrown
2024-01-17 12:42     ` Jeff Layton
2024-01-16 19:46 ` [PATCH 14/20] filelock: convert fl_blocker to file_lock_core Jeff Layton
2024-01-16 19:46 ` [PATCH 15/20] filelock: clean up locks_delete_block internals Jeff Layton
2024-01-16 19:46 ` [PATCH 16/20] filelock: reorganize locks_delete_block and __locks_insert_block Jeff Layton
2024-01-16 19:46 ` [PATCH 17/20] filelock: make assign_type helper take a file_lock_core pointer Jeff Layton
2024-01-16 19:46 ` [PATCH 18/20] filelock: convert locks_wake_up_blocks to " Jeff Layton
2024-01-16 19:46 ` [PATCH 19/20] filelock: convert locks_insert_lock_ctx and locks_delete_lock_ctx Jeff Layton
2024-01-16 19:46 ` [PATCH 20/20] filelock: split leases out of struct file_lock Jeff Layton
2024-01-16 22:44   ` NeilBrown
2024-01-17 12:45     ` Jeff Layton
2024-01-17 12:48 ` [PATCH 00/20] filelock: split struct file_lock into file_lock and file_lease structs Christian Brauner
2024-01-17 12:59   ` Jeff Layton
2024-01-17 13:25 ` [PATCH 02/20] filelock: add coccinelle scripts to move fields to struct file_lock_core David Howells
2024-01-17 13:40   ` Jeff Layton
2024-01-17 15:12 ` [PATCH 00/20] filelock: split struct file_lock into file_lock and file_lease structs Chuck Lever
2024-01-17 17:32   ` Jeff Layton
2024-01-17 18:59   ` NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6d7f8219f761bec5661906de80cbbb9b75db900d.camel@kernel.org \
    --to=jlayton@kernel.org \
    --cc=Dai.Ngo@oracle.com \
    --cc=aahringo@redhat.com \
    --cc=agruenba@redhat.com \
    --cc=anna@kernel.org \
    --cc=asmadeus@codewreck.org \
    --cc=brauner@kernel.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=chuck.lever@oracle.com \
    --cc=dhowells@redhat.com \
    --cc=ericvh@kernel.org \
    --cc=gfs2@lists.linux.dev \
    --cc=idryomov@gmail.com \
    --cc=jack@suse.cz \
    --cc=jlbec@evilplan.org \
    --cc=joseph.qi@linux.alibaba.com \
    --cc=kolga@netapp.com \
    --cc=linkinjeon@kernel.org \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=linux_oss@crudebyte.com \
    --cc=lsahlber@redhat.com \
    --cc=lucho@ionkov.net \
    --cc=marc.dionne@auristor.com \
    --cc=mark@fasheh.com \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=miklos@szeredi.hu \
    --cc=neilb@suse.de \
    --cc=ocfs2-devel@lists.linux.dev \
    --cc=pc@manguebit.com \
    --cc=rostedt@goodmis.org \
    --cc=samba-technical@lists.samba.org \
    --cc=senozhatsky@chromium.org \
    --cc=sfrench@samba.org \
    --cc=sprasad@microsoft.com \
    --cc=teigland@redhat.com \
    --cc=tom@talpey.com \
    --cc=trond.myklebust@hammerspace.com \
    --cc=v9fs@lists.linux.dev \
    --cc=viro@zeniv.linux.org.uk \
    --cc=xiubli@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).