ext2: compare old and new mode before setting update_mode flag

Message ID 20181117090100.29063-1-cgxu519@gmx.com
State Not Applicable
Headers show
Series
  • ext2: compare old and new mode before setting update_mode flag
Related show

Commit Message

cgxu519 Nov. 17, 2018, 9:01 a.m.
If new mode is the same as old mode we don't have to reset
inode mode in the rest of the code, so compare old and new
mode before setting update_mode flag.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/ext2/acl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jan Kara Nov. 19, 2018, 10:04 a.m. | #1
On Sat 17-11-18 17:01:00, Chengguang Xu wrote:
> If new mode is the same as old mode we don't have to reset
> inode mode in the rest of the code, so compare old and new
> mode before setting update_mode flag.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>

I don't think this is quite correct. E.g. I would think that i_ctime should
be updated even if the effective mode resulting from acl did not change.

								Honza

> ---
>  fs/ext2/acl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> index cf4c77f8dd08..f4dd728393c8 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -226,7 +226,8 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  		error = posix_acl_update_mode(inode, &mode, &acl);
>  		if (error)
>  			return error;
> -		update_mode = 1;
> +		if (mode != inode->i_mode)
> +			update_mode = 1;
>  	}
>  	error = __ext2_set_acl(inode, acl, type);
>  	if (!error && update_mode) {
> -- 
> 2.17.2
> 
>
cgxu519 Nov. 21, 2018, 10:55 a.m. | #2
On 11/19/18 6:04 PM, Jan Kara wrote:
> On Sat 17-11-18 17:01:00, Chengguang Xu wrote:
>> If new mode is the same as old mode we don't have to reset
>> inode mode in the rest of the code, so compare old and new
>> mode before setting update_mode flag.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> I don't think this is quite correct. E.g. I would think that i_ctime should
> be updated even if the effective mode resulting from acl did not change.

I thinkĀ  __ext2_set_acl() will probably update i_ctime in this case, am 
I missing something?

Thanks,


> ---
>   fs/ext2/acl.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> index cf4c77f8dd08..f4dd728393c8 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -226,7 +226,8 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>   		error = posix_acl_update_mode(inode, &mode, &acl);
>   		if (error)
>   			return error;
> -		update_mode = 1;
> +		if (mode != inode->i_mode)
> +			update_mode = 1;
>   	}
>   	error = __ext2_set_acl(inode, acl, type);
>   	if (!error && update_mode) {
> -- 
> 2.17.2
>
>
Jan Kara Nov. 21, 2018, 2:29 p.m. | #3
On Wed 21-11-18 18:55:28, cgxu519 wrote:
> On 11/19/18 6:04 PM, Jan Kara wrote:
> > On Sat 17-11-18 17:01:00, Chengguang Xu wrote:
> > > If new mode is the same as old mode we don't have to reset
> > > inode mode in the rest of the code, so compare old and new
> > > mode before setting update_mode flag.
> > > 
> > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > I don't think this is quite correct. E.g. I would think that i_ctime should
> > be updated even if the effective mode resulting from acl did not change.
> 
> I thinkĀ  __ext2_set_acl() will probably update i_ctime in this case, am I
> missing something?

Yeah, you're right. But I'd still prefer to keep the code simple as is.
There's no great reason to optimize this since saved time is going to be
minimal and the code is not performance critical.

								Honza


> > diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> > index cf4c77f8dd08..f4dd728393c8 100644
> > --- a/fs/ext2/acl.c
> > +++ b/fs/ext2/acl.c
> > @@ -226,7 +226,8 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >   		error = posix_acl_update_mode(inode, &mode, &acl);
> >   		if (error)
> >   			return error;
> > -		update_mode = 1;
> > +		if (mode != inode->i_mode)
> > +			update_mode = 1;
> >   	}
> >   	error = __ext2_set_acl(inode, acl, type);
> >   	if (!error && update_mode) {
> > -- 
> > 2.17.2
> > 
> >

Patch

diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
index cf4c77f8dd08..f4dd728393c8 100644
--- a/fs/ext2/acl.c
+++ b/fs/ext2/acl.c
@@ -226,7 +226,8 @@  ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		error = posix_acl_update_mode(inode, &mode, &acl);
 		if (error)
 			return error;
-		update_mode = 1;
+		if (mode != inode->i_mode)
+			update_mode = 1;
 	}
 	error = __ext2_set_acl(inode, acl, type);
 	if (!error && update_mode) {