diff mbox series

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

Message ID 154138144777.31651.4516223215037533891.stgit@noble
State New, archived
Headers show
Series Series short description | expand

Commit Message

NeilBrown Nov. 5, 2018, 1:30 a.m. UTC
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(-)

Comments

Jeff Layton Nov. 5, 2018, 12:18 p.m. UTC | #1
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.

> +		request.fl_flags = FL_FLOCK;
> +		locks_lock_file_wait(file, &request);
>  		gfs2_glock_dq(fl_gh);
>  		gfs2_holder_reinit(state, flags, fl_gh);
>  	} else {
> 
>
NeilBrown Nov. 6, 2018, 1:48 a.m. UTC | #2
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.

Do you want me to resend the series, to will you just update those
patches.

Sorry about that,
NeilBrown


>
>> +		request.fl_flags = FL_FLOCK;
>> +		locks_lock_file_wait(file, &request);
>>  		gfs2_glock_dq(fl_gh);
>>  		gfs2_holder_reinit(state, flags, fl_gh);
>>  	} else {
>> 
>> 
>
> -- 
> Jeff Layton <jlayton@kernel.org>
Jeff Layton Nov. 6, 2018, 1:20 p.m. UTC | #3
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!
diff mbox series

Patch

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;
+		request.fl_flags = FL_FLOCK;
+		locks_lock_file_wait(file, &request);
 		gfs2_glock_dq(fl_gh);
 		gfs2_holder_reinit(state, flags, fl_gh);
 	} else {