[PATCH] ext4: remove abnormal set for I_DATA_SEM subclass

[PATCH] ext4: remove abnormal set for I_DATA_SEM subclass

[Junil Lee]

The -EBUSY return value of dquot_enable() function means that just
want to update flags. If some users make a duplicate request to update
flags, lockdep could catch the false positive casued by needing to
allocate a quota block from inside ext4_map_blocks(), while holding
i_data_sem for a data inode. This results in this complaint:

       CPU0                    CPU1
       ----                    ----
  lock(&s->s_dquot.dqio_mutex);
                               lock(&ei->i_data_sem);
                               lock(&s->s_dquot.dqio_mutex);
  lock(&ei->i_data_sem);

Signed-off-by: Junil Lee <junil0814.lee@lge.com>
---
 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 13d2706..0757c9a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5637,7 +5637,7 @@ static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
 	lockdep_set_quota_inode(qf_inode, I_DATA_SEM_QUOTA);
 	err = dquot_enable(qf_inode, type, format_id, flags);
 	iput(qf_inode);
-	if (err)
+	if (err && err != -EBUSY)
 		lockdep_set_quota_inode(qf_inode, I_DATA_SEM_NORMAL);
 
 	return err;
-- 
2.6.2

Re: [PATCH] ext4: remove abnormal set for I_DATA_SEM subclass

[Theodore Y. Ts'o]

On Mon, Jul 09, 2018 at 04:58:28PM +0900, Junil Lee wrote:
> The -EBUSY return value of dquot_enable() function means that just
> want to update flags. If some users make a duplicate request to update
> flags, lockdep could catch the false positive casued by needing to
> allocate a quota block from inside ext4_map_blocks(), while holding
> i_data_sem for a data inode. This results in this complaint:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(&s->s_dquot.dqio_mutex);
>                                lock(&ei->i_data_sem);
>                                lock(&s->s_dquot.dqio_mutex);
>   lock(&ei->i_data_sem);

How does this happen in practice?  The function ext4_quota_enable() is
only called by ext4_enable_quotas(), and I don't see the code path
where this would happen.  And if it does it would be resulting an
EXT4-fs warning message getting printing indicating that a failure to
enable quotas with an error of EBUSY.  So how does this happen that
"users would make a duplicate request to update flags"?

       	     	    	      	      	 	- Ted

Re: [PATCH] ext4: remove abnormal set for I_DATA_SEM subclass

[Theodore Y. Ts'o]

On Mon, Jul 23, 2018 at 10:48:37AM +0900, 이준일/연구원/MC연구소 BSP실 BSP6팀(junil0814.lee@lge.com) wrote:
> Then, I have a question.
> quotactl() doesn't have case only to set limits flag, the routine to set
> the DQUOT_LIMITS_ENABLED flag is under dquot_enable() function.
> According to this logic, if users makes duplicate request to set
> DQUOT_LIMITS_ENABLED flags, can lockdep make the false alarm with ext4 ?

With the upstream kernel, if you call quotactl with the Q_QUOTAON
command, it will fail with EEXIST before it ever gets to the file
system specific quota code.  This happens in dquot_quota_enable() in
fs/quota/dquot.c.

I'm going to guess you didn't try to reproduce the problem with the
latest mainstream kernel, and then applied the patch, and verified the
problem went away before you submitted it?

Regards,

					- Ted

[PATCH] ext4: remove abnormal set for I_DATA_SEM subclass

[Junil Lee]

The -EBUSY return value of dquot_enable() function means that just
want to update flags. If some users make a duplicate request to update
flags, lockdep could catch the false positive casued by needing to
allocate a quota block from inside ext4_map_blocks(), while holding
i_data_sem for a data inode. This results in this comlaint:

       CPU0                    CPU1
       ----                    ----
  lock(&s->s_dquot.dqio_mutex);
                               lock(&ei->i_data_sem);
                               lock(&s->s_dquot.dqio_mutex);
  lock(&ei->i_data_sem);

Signed-off-by: Junil Lee <junil0814.lee@lge.com>
---
 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 13d2706..f16c92d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5637,7 +5637,7 @@ static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
 	lockdep_set_quota_inode(qf_inode, I_DATA_SEM_QUOTA);
 	err = dquot_enable(qf_inode, type, format_id, flags);
 	iput(qf_inode);
-	if (err)
+	if (err != -EBUSY)
 		lockdep_set_quota_inode(qf_inode, I_DATA_SEM_NORMAL);
 
 	return err;
-- 
2.6.2