[PATCH] ext4: Clear lockdep subtype for quota files on quota off

[PATCH] ext4: Clear lockdep subtype for quota files on quota off

[Jan Kara]

Quota files have special ranking of i_data_sem lock. We inform lockdep
about it when turning on quotas however when turning quotas off, we
don't clear the lockdep subclass from i_data_sem lock and thus when the
inode gets later reused for a normal file or directory, lockdep gets
confused and complains about possible deadlocks. Fix the problem by
resetting lockdep subclass of i_data_sem on quota off.

Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a9c72e39a4ee..77043dc7f704 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -846,14 +846,9 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
 {
 	int type;
 
-	if (ext4_has_feature_quota(sb)) {
-		dquot_disable(sb, -1,
-			      DQUOT_USAGE_ENABLED | DQUOT_LIMITS_ENABLED);
-	} else {
-		/* Use our quota_off function to clear inode flags etc. */
-		for (type = 0; type < EXT4_MAXQUOTAS; type++)
-			ext4_quota_off(sb, type);
-	}
+	/* Use our quota_off function to clear inode flags etc. */
+	for (type = 0; type < EXT4_MAXQUOTAS; type++)
+		ext4_quota_off(sb, type);
 }
 #else
 static inline void ext4_quota_off_umount(struct super_block *sb)
@@ -5476,7 +5471,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
 		goto out;
 
 	err = dquot_quota_off(sb, type);
-	if (err)
+	if (err || ext4_has_feature_quota(sb))
 		goto out_put;
 
 	inode_lock(inode);
@@ -5496,6 +5491,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
 out_unlock:
 	inode_unlock(inode);
 out_put:
+	lockdep_set_quota_inode(inode, I_DATA_SEM_NORMAL);
 	iput(inode);
 	return err;
 out:
-- 
2.12.0

Re: [PATCH] ext4: Clear lockdep subtype for quota files on quota off

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3562 bytes --]

On May 5, 2017, at 2:53 AM, Jan Kara <jack@suse.cz> wrote:
> 
> Quota files have special ranking of i_data_sem lock. We inform lockdep
> about it when turning on quotas however when turning quotas off, we
> don't clear the lockdep subclass from i_data_sem lock and thus when the
> inode gets later reused for a normal file or directory, lockdep gets
> confused and complains about possible deadlocks. Fix the problem by
> resetting lockdep subclass of i_data_sem on quota off.
> 
> Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Fixes: daf647d2dd58cec59570d7698a45b98e580f2076
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
---
The change to skip clearing the NOATIME/IMMUTABLE inode flags and times
on every mount/unmount when the journaled quota feature is enabled is
good, since it avoids needless overhead, but isn't really described in
the commit comment.  This isn't directly related to the lockdep, but
rather improving 957153fce8d2 "ext4: Set flags on quota files directly"
AFAICS.

It might be worthwhile to add a line to this commit like:

 Don't clear NOATIME/IMMUTABLE flags when journaled quota is enabled.

It also looks like ext4_quota_on() could use a similar check, to avoid
setting the NOATIME/IMMUTABLE flags on every mount if they are already
set.

I guess it isn't harmful to clear IMMUTABLE in the case of non-journaled
quotas, to maximize compatibility with older quota utilities, so long as
we don't incur this needless overhead for the newer journaled quota case.


There is a separate issue of what to do if the filesystem wasn't unmounted
cleanly and IMMUTABLE is still set?  If the userspace quotacheck always
clears IMMUTABLE when it is run, then there isn't much benefit in setting
the IMMUTABLE flag in the first place.  Is there some other way that the
userspace quota utilities know whether it is safe to update the quota files?

One possibility would be to use a similar open(O_EXCL) hack as we use
with block devices to avoid userspace trying to modify the quota file while
the kernel is using it?

Cheers, Andreas

> ---
> fs/ext4/super.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index a9c72e39a4ee..77043dc7f704 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -846,14 +846,9 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
> {
> 	int type;
> 
> -	if (ext4_has_feature_quota(sb)) {
> -		dquot_disable(sb, -1,
> -			      DQUOT_USAGE_ENABLED | DQUOT_LIMITS_ENABLED);
> -	} else {
> -		/* Use our quota_off function to clear inode flags etc. */
> -		for (type = 0; type < EXT4_MAXQUOTAS; type++)
> -			ext4_quota_off(sb, type);
> -	}
> +	/* Use our quota_off function to clear inode flags etc. */
> +	for (type = 0; type < EXT4_MAXQUOTAS; type++)
> +		ext4_quota_off(sb, type);
> }
> #else
> static inline void ext4_quota_off_umount(struct super_block *sb)
> @@ -5476,7 +5471,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
> 		goto out;
> 
> 	err = dquot_quota_off(sb, type);
> -	if (err)
> +	if (err || ext4_has_feature_quota(sb))
> 		goto out_put;
> 
> 	inode_lock(inode);
> @@ -5496,6 +5491,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
> out_unlock:
> 	inode_unlock(inode);
> out_put:
> +	lockdep_set_quota_inode(inode, I_DATA_SEM_NORMAL);
> 	iput(inode);
> 	return err;
> out:
> --
> 2.12.0
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ext4: Clear lockdep subtype for quota files on quota off

[Jan Kara]

On Fri 05-05-17 14:20:38, Andreas Dilger wrote:
> On May 5, 2017, at 2:53 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > Quota files have special ranking of i_data_sem lock. We inform lockdep
> > about it when turning on quotas however when turning quotas off, we
> > don't clear the lockdep subclass from i_data_sem lock and thus when the
> > inode gets later reused for a normal file or directory, lockdep gets
> > confused and complains about possible deadlocks. Fix the problem by
> > resetting lockdep subclass of i_data_sem on quota off.
> > 
> > Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Fixes: daf647d2dd58cec59570d7698a45b98e580f2076
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Thanks I'll add these tags.

> The change to skip clearing the NOATIME/IMMUTABLE inode flags and times
> on every mount/unmount when the journaled quota feature is enabled is
> good, since it avoids needless overhead, but isn't really described in
> the commit comment.  This isn't directly related to the lockdep, but
> rather improving 957153fce8d2 "ext4: Set flags on quota files directly"
> AFAICS.

I don't think I'm changing anything in that code actually. Previously, we
didn't call ext4_quota_off() at all when quota files were stored in system
inodes, now we do call it but just skip NOATIME/IMMUTABLE flag handling. So
what you write below is not related to this patch.

> It might be worthwhile to add a line to this commit like:
> 
>  Don't clear NOATIME/IMMUTABLE flags when journaled quota is enabled.
> 
> It also looks like ext4_quota_on() could use a similar check, to avoid
> setting the NOATIME/IMMUTABLE flags on every mount if they are already
> set.

I don't think it is worth it. Firstly, In don't think people care about
quotaon() performance that much, secondly those flags are expected to be
clear, thirdly, that function is called only for legacy quota files.

> I guess it isn't harmful to clear IMMUTABLE in the case of non-journaled
> quotas, to maximize compatibility with older quota utilities, so long as
> we don't incur this needless overhead for the newer journaled quota case.

We manipulate inode flags only for legacy quota files. For quota files that
are hidden system inodes, there's no point so we avoid that.

> There is a separate issue of what to do if the filesystem wasn't unmounted
> cleanly and IMMUTABLE is still set?  If the userspace quotacheck always
> clears IMMUTABLE when it is run, then there isn't much benefit in setting
> the IMMUTABLE flag in the first place.  Is there some other way that the
> userspace quota utilities know whether it is safe to update the quota files?

Userspace quotacheck does remove IMMUTABLE flag if it sees it. But it
checks using quotactl() calls whether quota isn't turned on on that
filesystem before doing that. Generally quota-tools take care and check
using quotactl() whether quota isn't turned on before going and modifying
quota files directly. However there are some Perl modules out there
modifying quota files which are not as careful...

> One possibility would be to use a similar open(O_EXCL) hack as we use
> with block devices to avoid userspace trying to modify the quota file while
> the kernel is using it?

Well, that would be possible and possibly more reliable than IMMUTABLE
flag. However since the problem is there only for legacy quota files, I
don't think it's worth the de fact API change. Hidden quota files solve all
these problems (and couple others) nicely...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: Clear lockdep subtype for quota files on quota off

[Ross Zwisler]

On Fri, May 05, 2017 at 10:53:27AM +0200, Jan Kara wrote:
> Quota files have special ranking of i_data_sem lock. We inform lockdep
> about it when turning on quotas however when turning quotas off, we
> don't clear the lockdep subclass from i_data_sem lock and thus when the
> inode gets later reused for a normal file or directory, lockdep gets
> confused and complains about possible deadlocks. Fix the problem by
> resetting lockdep subclass of i_data_sem on quota off.
> 
> Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Yep, this fixes the lockdep warning for me, thanks!

Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

> ---
>  fs/ext4/super.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index a9c72e39a4ee..77043dc7f704 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -846,14 +846,9 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
>  {
>  	int type;
>  
> -	if (ext4_has_feature_quota(sb)) {
> -		dquot_disable(sb, -1,
> -			      DQUOT_USAGE_ENABLED | DQUOT_LIMITS_ENABLED);
> -	} else {
> -		/* Use our quota_off function to clear inode flags etc. */
> -		for (type = 0; type < EXT4_MAXQUOTAS; type++)
> -			ext4_quota_off(sb, type);
> -	}
> +	/* Use our quota_off function to clear inode flags etc. */
> +	for (type = 0; type < EXT4_MAXQUOTAS; type++)
> +		ext4_quota_off(sb, type);
>  }
>  #else
>  static inline void ext4_quota_off_umount(struct super_block *sb)
> @@ -5476,7 +5471,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
>  		goto out;
>  
>  	err = dquot_quota_off(sb, type);
> -	if (err)
> +	if (err || ext4_has_feature_quota(sb))
>  		goto out_put;
>  
>  	inode_lock(inode);
> @@ -5496,6 +5491,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
>  out_unlock:
>  	inode_unlock(inode);
>  out_put:
> +	lockdep_set_quota_inode(inode, I_DATA_SEM_NORMAL);
>  	iput(inode);
>  	return err;
>  out:
> -- 
> 2.12.0
>