Ocfs2 devel - Feb 2018 - [PATCH 2/6] ocfs2: Fix quota recovery failure on unmount

Hello,

this patch series fixes various issues I've found with ocfs2 quota recovery.
There were possible quota recovery failures or deadlocks when it raced with
umount in a wrong way. Also quota recovery would fail if the filesystem was
mounted read-only. The final patch is an unrelated fix for mount option
parsing I've found when testing my changes. Warning! I've tested these
patches only in local mode (as I don't have ocfs2 clustered setup available
for testing). Can someone please verify things also work properly in clustered
mode when recovery of quotas from other nodes happens? Thanks!

								Honza

From: Linus Torvalds <torvalds at linux-foundation.org>

---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d9cf3a40eda9..659a7780aeb3 100644
--- a/Makefile
+++ b/Makefile
@@ -2,7 +2,7 @@
 VERSION = 4
 PATCHLEVEL = 16
 SUBLEVEL = 0
-EXTRAVERSION = -rc2
+EXTRAVERSION = -rc3
 NAME = Fearless Coyote
 
 # *DOCUMENTATION*
-- 
2.13.6

When filesystem is unmounted while there is still some recovery work
going on, it can happen that quotas get disabled before quota recovery
is complete resulting in failed quota recovery and inconsistent quota
accounting. Move disabling of recovery in ocfs2_dismount_volume() before
disabling of quotas to fix this race.

Signed-off-by: Jan Kara <jack at suse.cz>
---
 fs/ocfs2/super.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index ffa4952d432b..14c3d5ee6e24 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1904,6 +1904,13 @@ static void ocfs2_dismount_volume(struct super_block *sb,
int mnt_err)
 	/* Orphan scan should be stopped as early as possible */
 	ocfs2_orphan_scan_stop(osb);
 
+	/*
+	 * This will disable recovery and flush any recovery work. This needs
+	 * to happen before disabling quotas as quota recovery needs quotas
+	 * enabled.
+	 */
+	ocfs2_recovery_exit(osb);
+
 	ocfs2_disable_quotas(osb);
 
 	/* All dquots should be freed by now */
@@ -1915,9 +1922,6 @@ static void ocfs2_dismount_volume(struct super_block *sb,
int mnt_err)
 
 	ocfs2_truncate_log_shutdown(osb);
 
-	/* This will disable recovery and flush any recovery work. */
-	ocfs2_recovery_exit(osb);
-
 	ocfs2_journal_shutdown(osb);
 
 	ocfs2_sync_blockdev(sb);
-- 
2.13.6

When ocfs2 quota recovery races with umount, it can happen that umount
waits for recovery to finish while quota recovery waits to grab s_umount
semaphore held by umount. This results in a deadlock. Fix the problem by
not grabbing s_umount semaphore during quota recovery. We are protected
from disabling of quotas by the fact that quotas gets disabled only on
umount and that happens after recovery is disabled.

Reported-by: Shichangkuo <shi.changkuo at h3c.com>
Signed-off-by: Jan Kara <jack at suse.cz>
---
 fs/ocfs2/quota_local.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c
index 16c42ed0dca8..d60d744bbcb2 100644
--- a/fs/ocfs2/quota_local.c
+++ b/fs/ocfs2/quota_local.c
@@ -454,8 +454,10 @@ struct ocfs2_quota_recovery *ocfs2_begin_quota_recovery(
 
 /* Sync changes in local quota file into global quota file and
  * reinitialize local quota file.
- * The function expects local quota file to be already locked and
- * s_umount locked in shared mode. */
+ * The function expects local quota file to be already locked. Quota is
+ * disabled only during umount after disabling recovery so we are protected
+ * against that.
+ */
 static int ocfs2_recover_local_quota_file(struct inode *lqinode,
 					  int type,
 					  struct ocfs2_quota_recovery *rec)
@@ -586,7 +588,6 @@ int ocfs2_finish_quota_recovery(struct ocfs2_super *osb,
 {
 	unsigned int ino[OCFS2_MAXQUOTAS] = { LOCAL_USER_QUOTA_SYSTEM_INODE,
 					      LOCAL_GROUP_QUOTA_SYSTEM_INODE };
-	struct super_block *sb = osb->sb;
 	struct ocfs2_local_disk_dqinfo *ldinfo;
 	struct buffer_head *bh;
 	handle_t *handle;
@@ -598,7 +599,6 @@ int ocfs2_finish_quota_recovery(struct ocfs2_super *osb,
 	printk(KERN_NOTICE "ocfs2: Finishing quota recovery on device (%s) for
"
 	       "slot %u\n", osb->dev_str, slot_num);
 
-	down_read(&sb->s_umount);
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
 		if (list_empty(&(rec->r_list[type])))
 			continue;
@@ -675,7 +675,6 @@ int ocfs2_finish_quota_recovery(struct ocfs2_super *osb,
 			break;
 	}
 out:
-	up_read(&sb->s_umount);
 	kfree(rec);
 	return status;
 }
@@ -837,9 +836,10 @@ static int ocfs2_local_free_info(struct super_block *sb,
int type)
 	ocfs2_release_local_quota_bitmaps(&oinfo->dqi_chunk);
 
 	/*
-	 * s_umount held in exclusive mode protects us against racing with
-	 * recovery thread...
+	 * Recovery should be already disabled at this point so that we cannot
+	 * race with quota recovery.
 	 */
+	WARN_ON(!OCFS2_SB(sb)->disable_recovery);
 	if (oinfo->dqi_rec) {
 		ocfs2_free_quota_recovery(oinfo->dqi_rec);
 		mark_clean = 0;
-- 
2.13.6

Move scheduling of dqi_sync_work up in the call stack to
ocfs2_enable_quotas() and ocfs2_susp_quotas(). This will later allow us
to not schedule this work when quotas are enabled on read-only
filesystem and also makes scheduling & canceling of dqi_sync_work more
symmetric.

Signed-off-by: Jan Kara <jack at suse.cz>
---
 fs/ocfs2/quota_global.c |  3 ---
 fs/ocfs2/super.c        | 21 ++++++++++++++-------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
index 7a922190a8c7..0098914dde88 100644
--- a/fs/ocfs2/quota_global.c
+++ b/fs/ocfs2/quota_global.c
@@ -401,9 +401,6 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
 						OCFS2_QBLK_RESERVED_SPACE;
 	oinfo->dqi_gi.dqi_qtree_depth = qtree_depth(&oinfo->dqi_gi);
 	INIT_DELAYED_WORK(&oinfo->dqi_sync_work, qsync_work_fn);
-	schedule_delayed_work(&oinfo->dqi_sync_work,
-			      msecs_to_jiffies(oinfo->dqi_syncms));
-
 out_err:
 	return status;
 out_unlock:
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 14c3d5ee6e24..39b62569e7ff 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -910,22 +910,25 @@ static int ocfs2_susp_quotas(struct ocfs2_super *osb, int
unsuspend)
 					OCFS2_FEATURE_RO_COMPAT_USRQUOTA,
 					OCFS2_FEATURE_RO_COMPAT_GRPQUOTA};
 	int status = 0;
+	struct ocfs2_mem_dqinfo *oinfo;
 
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
 		if (!OCFS2_HAS_RO_COMPAT_FEATURE(sb, feature[type]))
 			continue;
-		if (unsuspend)
+		oinfo = sb_dqinfo(sb, type)->dqi_priv;
+		if (unsuspend) {
 			status = dquot_resume(sb, type);
-		else {
-			struct ocfs2_mem_dqinfo *oinfo;
-
+			if (status < 0)
+				break;
+			schedule_delayed_work(&oinfo->dqi_sync_work,
+					msecs_to_jiffies(oinfo->dqi_syncms));
+		} else {
 			/* Cancel periodic syncing before suspending */
-			oinfo = sb_dqinfo(sb, type)->dqi_priv;
 			cancel_delayed_work_sync(&oinfo->dqi_sync_work);
 			status = dquot_suspend(sb, type);
+			if (status < 0)
+				break;
 		}
-		if (status < 0)
-			break;
 	}
 	if (status < 0)
 		mlog(ML_ERROR, "Failed to suspend/unsuspend quotas on "
@@ -943,6 +946,7 @@ static int ocfs2_enable_quotas(struct ocfs2_super *osb)
 	unsigned int ino[OCFS2_MAXQUOTAS] = {
 					LOCAL_USER_QUOTA_SYSTEM_INODE,
 					LOCAL_GROUP_QUOTA_SYSTEM_INODE };
+	struct ocfs2_mem_dqinfo *oinfo;
 	int status;
 	int type;
 
@@ -960,6 +964,9 @@ static int ocfs2_enable_quotas(struct ocfs2_super *osb)
 				      DQUOT_USAGE_ENABLED);
 		if (status < 0)
 			goto out_quota_off;
+		oinfo = sb_dqinfo(sb, type)->dqi_priv;
+		schedule_delayed_work(&oinfo->dqi_sync_work,
+				      msecs_to_jiffies(oinfo->dqi_syncms));
 	}
 
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++)
-- 
2.13.6

Currently quotas are either disabled or suspended when the filesystem
is mounted read only. This results in quota recovery failing when in
happens on such mount and as a result quota accounting is wrong. Fix the
problem by enabling quotas even for read-only mounts. We just don't
start periodic flushing of our local changes to the global quota file
since that is pointless for read-only mounts.

Signed-off-by: Jan Kara <jack at suse.cz>
---
 fs/ocfs2/super.c | 99 +++++++++++++++++++++++++-------------------------------
 1 file changed, 44 insertions(+), 55 deletions(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 39b62569e7ff..af4481b98c65 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -135,7 +135,8 @@ static int ocfs2_get_sector(struct super_block *sb,
 			    int sect_size);
 static struct inode *ocfs2_alloc_inode(struct super_block *sb);
 static void ocfs2_destroy_inode(struct inode *inode);
-static int ocfs2_susp_quotas(struct ocfs2_super *osb, int unsuspend);
+static void ocfs2_enable_quota_sync(struct ocfs2_super *osb);
+static void ocfs2_disable_quota_sync(struct ocfs2_super *osb);
 static int ocfs2_enable_quotas(struct ocfs2_super *osb);
 static void ocfs2_disable_quotas(struct ocfs2_super *osb);
 
@@ -675,12 +676,9 @@ static int ocfs2_remount(struct super_block *sb, int
*flags, char *data)
 
 	/* We're going to/from readonly mode. */
 	if ((bool)(*flags & SB_RDONLY) != sb_rdonly(sb)) {
-		/* Disable quota accounting before remounting RO */
-		if (*flags & SB_RDONLY) {
-			ret = ocfs2_susp_quotas(osb, 0);
-			if (ret < 0)
-				goto out;
-		}
+		/* Disable quota syncing before remounting RO */
+		if (*flags & SB_RDONLY)
+			ocfs2_disable_quota_sync(osb);
 		/* Lock here so the check of HARD_RO and the potential
 		 * setting of SOFT_RO is atomic. */
 		spin_lock(&osb->osb_lock);
@@ -714,21 +712,9 @@ static int ocfs2_remount(struct super_block *sb, int
*flags, char *data)
 		trace_ocfs2_remount(sb->s_flags, osb->osb_flags, *flags);
 unlock_osb:
 		spin_unlock(&osb->osb_lock);
-		/* Enable quota accounting after remounting RW */
-		if (!ret && !(*flags & SB_RDONLY)) {
-			if (sb_any_quota_suspended(sb))
-				ret = ocfs2_susp_quotas(osb, 1);
-			else
-				ret = ocfs2_enable_quotas(osb);
-			if (ret < 0) {
-				/* Return back changes... */
-				spin_lock(&osb->osb_lock);
-				sb->s_flags |= SB_RDONLY;
-				osb->osb_flags |= OCFS2_OSB_SOFT_RO;
-				spin_unlock(&osb->osb_lock);
-				goto out;
-			}
-		}
+		/* Enable quota syncing after remounting RW */
+		if (!ret && !(*flags & SB_RDONLY))
+			ocfs2_enable_quota_sync(osb);
 	}
 
 	if (!ret) {
@@ -902,38 +888,33 @@ static int ocfs2_verify_userspace_stack(struct ocfs2_super
*osb,
 	return 0;
 }
 
-static int ocfs2_susp_quotas(struct ocfs2_super *osb, int unsuspend)
+static void ocfs2_enable_quota_sync(struct ocfs2_super *osb)
 {
 	int type;
 	struct super_block *sb = osb->sb;
-	unsigned int feature[OCFS2_MAXQUOTAS] = {
-					OCFS2_FEATURE_RO_COMPAT_USRQUOTA,
-					OCFS2_FEATURE_RO_COMPAT_GRPQUOTA};
-	int status = 0;
 	struct ocfs2_mem_dqinfo *oinfo;
 
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
-		if (!OCFS2_HAS_RO_COMPAT_FEATURE(sb, feature[type]))
+		if (!sb_has_quota_active(sb, type))
 			continue;
 		oinfo = sb_dqinfo(sb, type)->dqi_priv;
-		if (unsuspend) {
-			status = dquot_resume(sb, type);
-			if (status < 0)
-				break;
-			schedule_delayed_work(&oinfo->dqi_sync_work,
-					msecs_to_jiffies(oinfo->dqi_syncms));
-		} else {
-			/* Cancel periodic syncing before suspending */
-			cancel_delayed_work_sync(&oinfo->dqi_sync_work);
-			status = dquot_suspend(sb, type);
-			if (status < 0)
-				break;
-		}
+		schedule_delayed_work(&oinfo->dqi_sync_work,
+				      msecs_to_jiffies(oinfo->dqi_syncms));
+	}
+}
+
+static void ocfs2_disable_quota_sync(struct ocfs2_super *osb)
+{
+	int type;
+	struct super_block *sb = osb->sb;
+	struct ocfs2_mem_dqinfo *oinfo;
+
+	for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
+		if (!sb_has_quota_active(sb, type))
+			continue;
+		oinfo = sb_dqinfo(sb, type)->dqi_priv;
+		cancel_delayed_work_sync(&oinfo->dqi_sync_work);
 	}
-	if (status < 0)
-		mlog(ML_ERROR, "Failed to suspend/unsuspend quotas on "
-		     "remount (error = %d).\n", status);
-	return status;
 }
 
 static int ocfs2_enable_quotas(struct ocfs2_super *osb)
@@ -946,11 +927,18 @@ static int ocfs2_enable_quotas(struct ocfs2_super *osb)
 	unsigned int ino[OCFS2_MAXQUOTAS] = {
 					LOCAL_USER_QUOTA_SYSTEM_INODE,
 					LOCAL_GROUP_QUOTA_SYSTEM_INODE };
-	struct ocfs2_mem_dqinfo *oinfo;
 	int status;
 	int type;
+	bool ro;
 
 	sb_dqopt(sb)->flags |= DQUOT_QUOTA_SYS_FILE | DQUOT_NEGATIVE_USAGE;
+	/*
+	 * We temporarily switch sb to read-write to allow quota setup to pass
+ 	 * cleanly
+	 */
+	ro = sb_rdonly(sb);
+	if (ro)
+		sb->s_flags &= ~SB_RDONLY;
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
 		if (!OCFS2_HAS_RO_COMPAT_FEATURE(sb, feature[type]))
 			continue;
@@ -964,16 +952,19 @@ static int ocfs2_enable_quotas(struct ocfs2_super *osb)
 				      DQUOT_USAGE_ENABLED);
 		if (status < 0)
 			goto out_quota_off;
-		oinfo = sb_dqinfo(sb, type)->dqi_priv;
-		schedule_delayed_work(&oinfo->dqi_sync_work,
-				      msecs_to_jiffies(oinfo->dqi_syncms));
 	}
 
+	if (!ro)
+		ocfs2_enable_quota_sync(osb);
+	else
+		sb->s_flags |= SB_RDONLY;
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++)
 		iput(inode[type]);
 	return 0;
 out_quota_off:
 	ocfs2_disable_quotas(osb);
+	if (ro)
+		sb->s_flags |= SB_RDONLY;
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++)
 		iput(inode[type]);
 	mlog_errno(status);
@@ -985,15 +976,13 @@ static void ocfs2_disable_quotas(struct ocfs2_super *osb)
 	int type;
 	struct inode *inode;
 	struct super_block *sb = osb->sb;
-	struct ocfs2_mem_dqinfo *oinfo;
 
 	/* We mostly ignore errors in this function because there's not much
 	 * we can do when we see them */
+	ocfs2_disable_quota_sync(osb);
 	for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
 		if (!sb_has_quota_loaded(sb, type))
 			continue;
-		oinfo = sb_dqinfo(sb, type)->dqi_priv;
-		cancel_delayed_work_sync(&oinfo->dqi_sync_work);
 		inode = igrab(sb->s_dquot.files[type]);
 		/* Turn off quotas. This will remove all dquot structures from
 		 * memory and so they will be automatically synced to global
@@ -1185,7 +1174,7 @@ static int ocfs2_fill_super(struct super_block *sb, void
*data, int silent)
 	/* Now we can initialize quotas because we can afford to wait
 	 * for cluster locks recovery now. That also means that truncation
 	 * log recovery can happen but that waits for proper quota setup */
-	if (!sb_rdonly(sb)) {
+	if (!ocfs2_is_hard_readonly(osb)) {
 		status = ocfs2_enable_quotas(osb);
 		if (status < 0) {
 			/* We have to err-out specially here because
@@ -1195,9 +1184,9 @@ static int ocfs2_fill_super(struct super_block *sb, void
*data, int silent)
 			wake_up(&osb->osb_mount_event);
 			return status;
 		}
-	}
 
-	ocfs2_complete_quota_recovery(osb);
+		ocfs2_complete_quota_recovery(osb);
+	}
 
 	/* Now we wake up again for processes waiting for quotas */
 	atomic_set(&osb->vol_state, VOLUME_MOUNTED_QUOTAS);
-- 
2.13.6

When a filesystem is mounted without any options, the mount succeeds
however remounting is later not possible as the kernel complains:

(mount,568,0):ocfs2_remount:657 ERROR: Cannot change heartbeat mode on
remount

The problem is that in this case no heartbeat option got set and so
remount with heartbeat=none as shown in /proc/mounts is considered
to be a change of hearbeat mode.

Fix the problem by defaulting heartbeat mode to 'none' and making sure
it gets set when no mount options are specified.

Signed-off-by: Jan Kara <jack at suse.cz>
---
 fs/ocfs2/super.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index af4481b98c65..45d59766a357 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1289,10 +1289,8 @@ static int ocfs2_parse_options(struct super_block *sb,
 	mopt->resv_level = OCFS2_DEFAULT_RESV_LEVEL;
 	mopt->dir_resv_level = -1;
 
-	if (!options) {
-		status = 1;
-		goto bail;
-	}
+	if (!options)
+		goto check_opts;
 
 	while ((p = strsep(&options, ",")) != NULL) {
 		if (!*p)
@@ -1486,15 +1484,21 @@ static int ocfs2_parse_options(struct super_block *sb,
 		}
 	}
 
+check_opts:
 	if (user_stack == 0) {
+		int weight;
+
 		/* Ensure only one heartbeat mode */
 		tmp = mopt->mount_opt & (OCFS2_MOUNT_HB_LOCAL |
 					 OCFS2_MOUNT_HB_GLOBAL |
 					 OCFS2_MOUNT_HB_NONE);
-		if (hweight32(tmp) != 1) {
+		weight = hweight32(tmp);
+		if (weight > 1) {
 			mlog(ML_ERROR, "Invalid heartbeat mount options\n");
 			status = 0;
 			goto bail;
+		} else if (weight == 0) {
+			mopt->mount_opt |= OCFS2_MOUNT_HB_NONE;
 		}
 	}
 
-- 
2.13.6

Hi Jan,

On 2018/2/28 19:18, Jan Kara wrote:
> Hello,
> 
> this patch series fixes various issues I've found with ocfs2 quota
recovery.
> There were possible quota recovery failures or deadlocks when it raced with
> umount in a wrong way. Also quota recovery would fail if the filesystem was
> mounted read-only. The final patch is an unrelated fix for mount option
> parsing I've found when testing my changes. Warning! I've tested
these
> patches only in local mode (as I don't have ocfs2 clustered setup
available
> for testing). Can someone please verify things also work properly in
clustered
> mode when recovery of quotas from other nodes happens? Thanks!
We will perform a test applying your patch set in cluster mode ocfs2 and give a 
feedback later.

Thanks,
Changwei
> 
> 								Honza
>