| Message ID | 20170505085327.27103-1-jack@suse.cz |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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 >
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
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:
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(-)