[Ocfs2-devel] [PATCH 0/6] ocfs2: Fix various quota recovery

[Ocfs2-devel] [PATCH 0/6] ocfs2: Fix various quota recovery

[Jan Kara]

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

[Ocfs2-devel] [PATCH 1/6] Linux 4.16-rc3

[Jan Kara]

From: Linus Torvalds <torvalds@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

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

[Jan Kara]

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@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

[Ocfs2-devel] [PATCH 3/6] ocfs2: Fix deadlock during umount

[Jan Kara]

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@h3c.com>
Signed-off-by: Jan Kara <jack@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

[Ocfs2-devel] [PATCH 4/6] ocfs2: Move scheduling of dqi_sync_work up in call stack

[Jan Kara]

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@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

[Ocfs2-devel] [PATCH 5/6] ocfs2: Fix quota recovery for read-only mounts

[Jan Kara]

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@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

[Ocfs2-devel] [PATCH 6/6] ocfs2: Do not fail remount when mounted without heartbeat option

[Jan Kara]

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@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

[Ocfs2-devel] [PATCH 0/6] ocfs2: Fix various quota recovery

[Changwei Ge]

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
> 

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

[Shichangkuo]

Hi Jan,
    This patch will cause kernel crashes while umounting ocfs2.
    After parsing the core file, I find the root reason is as follows:
    osb->recovery_map is freed when recovery thread exited, but following functions still access it by calling ocfs2_inode_lock.
    So, we should disable quotas between ocfs2_truncate_log_shutdown and ocfs2_recovery_exit.
    Am I right?

Thanks
Changkuo

>>>
> 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@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

-------------------------------------------------------------------------------------------------------------------------------------
?????????????????????????????????????
????????????????????????????????????????
????????????????????????????????????????
???
This e-mail and its attachments contain confidential information from New H3C, which is
intended only for the person or entity whose address is listed above. Any use of the
information contained herein in any way (including, but not limited to, total or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
by phone or email immediately and delete it!

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

[Jan Kara]

Hi Changkuo!

On Mon 19-03-18 09:02:42, Shichangkuo wrote:
> Hi Jan,
>     This patch will cause kernel crashes while umounting ocfs2.
>     After parsing the core file, I find the root reason is as follows:
>     osb->recovery_map is freed when recovery thread exited, but following functions still access it by calling ocfs2_inode_lock.
>     So, we should disable quotas between ocfs2_truncate_log_shutdown and ocfs2_recovery_exit.
>     Am I right?

Thanks a lot for testing and the analysis. Sadly fixing this is not that
easy. We have to have recovery thread still working so that we can acquire
locks to sync and disable quotas. OTOH we need to stop recovery thread from
racing of quota recovery with disabling of quotas. Drat. I need to go back
and rethink this...

								Honza

> 
> Thanks
> Changkuo
> 
> >>>
> > 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@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
> 
> -------------------------------------------------------------------------------------------------------------------------------------
> ?????????????????????????????????????
> ????????????????????????????????????????
> ????????????????????????????????????????
> ???
> This e-mail and its attachments contain confidential information from New H3C, which is
> intended only for the person or entity whose address is listed above. Any use of the
> information contained herein in any way (including, but not limited to, total or partial
> disclosure, reproduction, or dissemination) by persons other than the intended
> recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
> by phone or email immediately and delete it!
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR