Re: [PATCH 04/12] gfs2: properly initial file_lock used for unlock.

From: Jeff Layton <jlayton@kernel.org>
To: NeilBrown <neilb@suse.com>, Alexander Viro <viro@zeniv.linux.org.uk>
Cc: "J. Bruce Fields" <bfields@fieldses.org>,
	Martin Wilck <mwilck@suse.de>,
	linux-fsdevel@vger.kernel.org,
	Frank Filz <ffilzlnx@mindspring.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/12] gfs2: properly initial file_lock used for unlock.
Date: Tue, 06 Nov 2018 08:20:42 -0500	[thread overview]
Message-ID: <60e81aebf4e76c6eafe90618a3e1f04a0ee3de87.camel@kernel.org> (raw)
In-Reply-To: <875zxbyo19.fsf@notabene.neil.brown.name>

On Tue, 2018-11-06 at 12:48 +1100, NeilBrown wrote:
> On Mon, Nov 05 2018, Jeff Layton wrote:
> 
> > On Mon, 2018-11-05 at 12:30 +1100, NeilBrown wrote:
> > > Rather than assuming all-zeros is sufficient, use the available API to
> > > initialize the file_lock structure use for unlock.
> > > VFS-level changes will soon make it important that the
> > > list_heads in file_lock are always properly initialized.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.com>
> > > ---
> > >  fs/gfs2/file.c |   10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> > > index 45a17b770d97..271f847705e3 100644
> > > --- a/fs/gfs2/file.c
> > > +++ b/fs/gfs2/file.c
> > > @@ -1199,13 +1199,13 @@ static int do_flock(struct file *file, int cmd, struct file_lock *fl)
> > >  	mutex_lock(&fp->f_fl_mutex);
> > >  
> > >  	if (gfs2_holder_initialized(fl_gh)) {
> > > +		struct file_lock request;
> > >  		if (fl_gh->gh_state == state)
> > >  			goto out;
> > > -		locks_lock_file_wait(file,
> > > -				     &(struct file_lock) {
> > > -					     .fl_type = F_UNLCK,
> > > -					     .fl_flags = FL_FLOCK
> > > -				     });
> > > +		locks_init_lock(&request);
> > > +		request.fl_type = F_UNLOCK;
> > 
> > F_UNLCK ?
> > 
> > The ocfs2 patch has the same bug.
> 
> Anyone would think that I hadn't even compile tested.....
> 
> This is true for OCFS2 :-( but I had actually compile-tested with GFS2
> enabled.
> But CONFIG_DLM *wasn't* enabled, so GFS2 was compiled without locking
> support.
> I guess there is a good reason that GFS2 doesn't require DLM.
> 

I think you can run GFS2 in a single-node configuration with local
locking.

> Do you want me to resend the series, to will you just update those
> patches.
> 
> Sorry about that,
> NeilBrown
> 

No worries. I'll just fix those patches up and note it in the
changelogs. Other than the build failure, this seems to be doing fine in
testing so far. I'll likely push them to linux-next later this week.

> > 
> > > +		request.fl_flags = FL_FLOCK;
> > > +		locks_lock_file_wait(file, &request);
> > >  		gfs2_glock_dq(fl_gh);
> > >  		gfs2_holder_reinit(state, flags, fl_gh);
> > >  	} else {
> > > 
> > > 

Thanks again!
-- 
Jeff Layton <jlayton@kernel.org>