[PATCHSET 2/6] fuse2fs: refactor unmount code

[PATCHSET 2/6] fuse2fs: refactor unmount code

[Darrick J. Wong]

Hi all,

In this series, we refactor the code around ext2fs_close to get ready
for iomap mode.  The significant part of this series is moving the
unmount code to op_destroy, because we want to release the block device
as a part of the umount(2) process to maintain expected behavior of the
in-kernel local filesystem drivers.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

Comments and questions are, as always, welcome.

e2fsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/e2fsprogs.git/log/?h=fuse2fs-refactor-unmounting
---
Commits in this patchset:
 * fuse2fs: get rid of the global_fs variable
 * fuse2fs: hoist lockfile code
 * fuse2fs: hoist unmount code from main
---
 misc/fuse2fs.c |  189 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 104 insertions(+), 85 deletions(-)

[PATCH 1/3] fuse2fs: get rid of the global_fs variable

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Get rid of this global variable now that we don't need it anywhere.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 misc/fuse2fs.c |   77 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 40 deletions(-)


diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index 6255d960bd802f..948a12c2812b65 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -122,8 +122,6 @@
 #endif
 #endif /* !defined(ENODATA) */
 
-static ext2_filsys global_fs; /* Try not to use this directly */
-
 static inline uint64_t round_up(uint64_t b, unsigned int align)
 {
 	unsigned int m;
@@ -1077,7 +1075,7 @@ static void *op_init(struct fuse_conn_info *conn
 		log_printf(ff, "%s %s.\n", _("mounted filesystem"), uuid);
 	}
 
-	if (global_fs->flags & EXT2_FLAG_RW)
+	if (ff->fs->flags & EXT2_FLAG_RW)
 		fuse2fs_read_bitmaps(ff);
 
 	return ff;
@@ -5005,14 +5003,13 @@ int main(int argc, char *argv[])
 	if (fctx.directio)
 		flags |= EXT2_FLAG_DIRECT_IO;
 	err = ext2fs_open2(fctx.device, options, flags, 0, 0, unix_io_manager,
-			   &global_fs);
+			   &fctx.fs);
 	if (err) {
 		err_printf(&fctx, "%s.\n", error_message(err));
 		err_printf(&fctx, "%s\n", _("Please run e2fsck -fy."));
 		goto out;
 	}
-	fctx.fs = global_fs;
-	global_fs->priv_data = &fctx;
+	fctx.fs->priv_data = &fctx;
 	fctx.blocklog = u_log2(fctx.fs->blocksize);
 	fctx.blockmask = fctx.fs->blocksize - 1;
 
@@ -5023,7 +5020,7 @@ int main(int argc, char *argv[])
 
 		snprintf(buf, sizeof(buf), "cache_blocks=%llu",
 			 FUSE2FS_B_TO_FSBT(&fctx, fctx.cache_size));
-		err = io_channel_set_options(global_fs->io, buf);
+		err = io_channel_set_options(fctx.fs->io, buf);
 		if (err) {
 			err_printf(&fctx, "%s %lluk: %s\n",
 				   _("cannot set disk cache size to"),
@@ -5035,24 +5032,24 @@ int main(int argc, char *argv[])
 
 	ret = 3;
 
-	if (ext2fs_has_feature_quota(global_fs->super)) {
+	if (ext2fs_has_feature_quota(fctx.fs->super)) {
 		err_printf(&fctx, "%s", _("quotas not supported."));
 		goto out;
 	}
-	if (ext2fs_has_feature_verity(global_fs->super)) {
+	if (ext2fs_has_feature_verity(fctx.fs->super)) {
 		err_printf(&fctx, "%s", _("verity not supported."));
 		goto out;
 	}
-	if (ext2fs_has_feature_encrypt(global_fs->super)) {
+	if (ext2fs_has_feature_encrypt(fctx.fs->super)) {
 		err_printf(&fctx, "%s", _("encryption not supported."));
 		goto out;
 	}
-	if (ext2fs_has_feature_casefold(global_fs->super)) {
+	if (ext2fs_has_feature_casefold(fctx.fs->super)) {
 		err_printf(&fctx, "%s", _("casefolding not supported."));
 		goto out;
 	}
 
-	if (global_fs->super->s_state & EXT2_ERROR_FS) {
+	if (fctx.fs->super->s_state & EXT2_ERROR_FS) {
 		err_printf(&fctx, "%s\n",
  _("Errors detected; running e2fsck is required."));
 		goto out;
@@ -5062,73 +5059,73 @@ int main(int argc, char *argv[])
 	 * ext4 can't do COW of shared blocks, so if the feature is enabled,
 	 * we must force ro mode.
 	 */
-	if (ext2fs_has_feature_shared_blocks(global_fs->super))
+	if (ext2fs_has_feature_shared_blocks(fctx.fs->super))
 		fctx.ro = 1;
 
-	if (ext2fs_has_feature_journal_needs_recovery(global_fs->super)) {
+	if (ext2fs_has_feature_journal_needs_recovery(fctx.fs->super)) {
 		if (fctx.norecovery) {
 			log_printf(&fctx, "%s\n",
  _("Mounting read-only without recovering journal."));
 			fctx.ro = 1;
-			global_fs->flags &= ~EXT2_FLAG_RW;
+			fctx.fs->flags &= ~EXT2_FLAG_RW;
 		} else {
 			log_printf(&fctx, "%s\n", _("Recovering journal."));
-			err = ext2fs_run_ext3_journal(&global_fs);
+			err = ext2fs_run_ext3_journal(&fctx.fs);
 			if (err) {
 				err_printf(&fctx, "%s.\n", error_message(err));
 				err_printf(&fctx, "%s\n",
 						_("Please run e2fsck -fy."));
 				goto out;
 			}
-			ext2fs_clear_feature_journal_needs_recovery(global_fs->super);
-			ext2fs_mark_super_dirty(global_fs);
+			ext2fs_clear_feature_journal_needs_recovery(fctx.fs->super);
+			ext2fs_mark_super_dirty(fctx.fs);
 		}
 	}
 
-	ret = fuse2fs_check_root_dir(global_fs);
+	ret = fuse2fs_check_root_dir(fctx.fs);
 	if (ret)
 		goto out;
 
-	if (global_fs->flags & EXT2_FLAG_RW) {
-		if (ext2fs_has_feature_journal(global_fs->super))
+	if (fctx.fs->flags & EXT2_FLAG_RW) {
+		if (ext2fs_has_feature_journal(fctx.fs->super))
 			log_printf(&fctx, "%s",
  _("Warning: fuse2fs does not support using the journal.\n"
    "There may be file system corruption or data loss if\n"
    "the file system is not gracefully unmounted.\n"));
 	}
 
-	if (!(global_fs->super->s_state & EXT2_VALID_FS))
+	if (!(fctx.fs->super->s_state & EXT2_VALID_FS))
 		err_printf(&fctx, "%s\n",
  _("Warning: Mounting unchecked fs, running e2fsck is recommended."));
-	if (global_fs->super->s_max_mnt_count > 0 &&
-	    global_fs->super->s_mnt_count >= global_fs->super->s_max_mnt_count)
+	if (fctx.fs->super->s_max_mnt_count > 0 &&
+	    fctx.fs->super->s_mnt_count >= fctx.fs->super->s_max_mnt_count)
 		err_printf(&fctx, "%s\n",
  _("Warning: Maximal mount count reached, running e2fsck is recommended."));
-	if (global_fs->super->s_checkinterval > 0 &&
-	    (time_t) (global_fs->super->s_lastcheck +
-		      global_fs->super->s_checkinterval) <= time(0))
+	if (fctx.fs->super->s_checkinterval > 0 &&
+	    (time_t) (fctx.fs->super->s_lastcheck +
+		      fctx.fs->super->s_checkinterval) <= time(0))
 		err_printf(&fctx, "%s\n",
  _("Warning: Check time reached; running e2fsck is recommended."));
-	if (global_fs->super->s_last_orphan)
+	if (fctx.fs->super->s_last_orphan)
 		err_printf(&fctx, "%s\n",
  _("Orphans detected; running e2fsck is recommended."));
 
 	/* Clear the valid flag so that an unclean shutdown forces a fsck */
-	if (global_fs->flags & EXT2_FLAG_RW) {
-		global_fs->super->s_mnt_count++;
-		ext2fs_set_tstamp(global_fs->super, s_mtime, time(NULL));
-		global_fs->super->s_state &= ~EXT2_VALID_FS;
-		ext2fs_mark_super_dirty(global_fs);
-		err = ext2fs_flush2(global_fs, 0);
+	if (fctx.fs->flags & EXT2_FLAG_RW) {
+		fctx.fs->super->s_mnt_count++;
+		ext2fs_set_tstamp(fctx.fs->super, s_mtime, time(NULL));
+		fctx.fs->super->s_state &= ~EXT2_VALID_FS;
+		ext2fs_mark_super_dirty(fctx.fs);
+		err = ext2fs_flush2(fctx.fs, 0);
 		if (err) {
-			translate_error(global_fs, 0, err);
+			translate_error(fctx.fs, 0, err);
 			ret |= 32;
 			goto out;
 		}
 	}
 
 	if (!fctx.errors_behavior)
-		fctx.errors_behavior = global_fs->super->s_errors;
+		fctx.errors_behavior = fctx.fs->super->s_errors;
 
 	/* Initialize generation counter */
 	get_random_bytes(&fctx.next_generation, sizeof(unsigned int));
@@ -5219,13 +5216,13 @@ int main(int argc, char *argv[])
  _("Mount failed while opening filesystem.  Check dmesg(1) for details."));
 		fflush(orig_stderr);
 	}
-	if (global_fs) {
-		err = ext2fs_close(global_fs);
+	if (fctx.fs) {
+		err = ext2fs_close(fctx.fs);
 		if (err) {
 			com_err(argv[0], err, "while closing fs");
-			ext2fs_free(global_fs);
+			ext2fs_free(fctx.fs);
 		}
-		global_fs = NULL;
+		fctx.fs = NULL;
 	}
 	if (fctx.lockfile) {
 		if (unlink(fctx.lockfile)) {

[PATCH 2/3] fuse2fs: hoist lockfile code

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Hoist the lockfile handling code into separate helpers before we start
rearranging the code that opens and closes filesystems.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 misc/fuse2fs.c |   94 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 41 deletions(-)


diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index 948a12c2812b65..a66c0496e66dcf 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -859,6 +859,54 @@ static int check_inum_access(struct fuse2fs *ff, ext2_ino_t ino, int mask)
 	return -EACCES;
 }
 
+static errcode_t fuse2fs_acquire_lockfile(struct fuse2fs *ff)
+{
+	char *resolved;
+	int lockfd;
+	errcode_t err;
+
+	lockfd = open(ff->lockfile, O_RDWR | O_CREAT | O_EXCL, 0400);
+	if (lockfd < 0) {
+		if (errno == EEXIST)
+			err = EWOULDBLOCK;
+		else
+			err = errno;
+		err_printf(ff, "%s: %s: %s\n", ff->lockfile,
+			   _("opening lockfile failed"),
+			   strerror(err));
+		ff->lockfile = NULL;
+		return err;
+	}
+	close(lockfd);
+
+	resolved = realpath(ff->lockfile, NULL);
+	if (!resolved) {
+		err = errno;
+		err_printf(ff, "%s: %s: %s\n", ff->lockfile,
+			   _("resolving lockfile failed"),
+			   strerror(err));
+		unlink(ff->lockfile);
+		ff->lockfile = NULL;
+		return err;
+	}
+	free(ff->lockfile);
+	ff->lockfile = resolved;
+
+	return 0;
+}
+
+static void fuse2fs_release_lockfile(struct fuse2fs *ff)
+{
+	if (unlink(ff->lockfile)) {
+		errcode_t err = errno;
+
+		err_printf(ff, "%s: %s: %s\n", ff->lockfile,
+			   _("removing lockfile failed"),
+			   strerror(err));
+	}
+	free(ff->lockfile);
+}
+
 static void op_destroy(void *p EXT2FS_ATTR((unused)))
 {
 	struct fuse2fs *ff = fuse2fs_get();
@@ -4962,38 +5010,9 @@ int main(int argc, char *argv[])
 		fctx.alloc_all_blocks = 1;
 	}
 
-	if (fctx.lockfile) {
-		char *resolved;
-		int lockfd;
-
-		lockfd = open(fctx.lockfile, O_RDWR | O_CREAT | O_EXCL, 0400);
-		if (lockfd < 0) {
-			if (errno == EEXIST)
-				err = EWOULDBLOCK;
-			else
-				err = errno;
-			err_printf(&fctx, "%s: %s: %s\n", fctx.lockfile,
-				   _("opening lockfile failed"),
-				   strerror(err));
-			fctx.lockfile = NULL;
-			ret |= 32;
-			goto out;
-		}
-		close(lockfd);
-
-		resolved = realpath(fctx.lockfile, NULL);
-		if (!resolved) {
-			err = errno;
-			err_printf(&fctx, "%s: %s: %s\n", fctx.lockfile,
-				   _("resolving lockfile failed"),
-				   strerror(err));
-			unlink(fctx.lockfile);
-			fctx.lockfile = NULL;
-			ret |= 32;
-			goto out;
-		}
-		free(fctx.lockfile);
-		fctx.lockfile = resolved;
+	if (fctx.lockfile && fuse2fs_acquire_lockfile(&fctx)) {
+		ret |= 32;
+		goto out;
 	}
 
 	/* Start up the fs (while we still can use stdout) */
@@ -5224,15 +5243,8 @@ int main(int argc, char *argv[])
 		}
 		fctx.fs = NULL;
 	}
-	if (fctx.lockfile) {
-		if (unlink(fctx.lockfile)) {
-			err = errno;
-			err_printf(&fctx, "%s: %s: %s\n", fctx.lockfile,
-				   _("removing lockfile failed"),
-				   strerror(err));
-		}
-		free(fctx.lockfile);
-	}
+	if (fctx.lockfile)
+		fuse2fs_release_lockfile(&fctx);
 	if (fctx.device)
 		free(fctx.device);
 	fuse_opt_free_args(&args);

[PATCH 3/3] fuse2fs: hoist unmount code from main

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Hoist the unmount code into a separate function so that we can reduce
the complexity of main().  This also sets us up for unmounting the
filesystem from op_destroy, which we'll need for fuse2fs+iomap mode to
maintain the expected behavior that the block device is free when
umount(8) returns.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 misc/fuse2fs.c |   30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)


diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index a66c0496e66dcf..23a1cd8d5d5d0a 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -907,6 +907,25 @@ static void fuse2fs_release_lockfile(struct fuse2fs *ff)
 	free(ff->lockfile);
 }
 
+static void fuse2fs_unmount(struct fuse2fs *ff)
+{
+	errcode_t err;
+
+	if (!ff->fs)
+		return;
+
+	err = ext2fs_close(ff->fs);
+	if (err) {
+		err_printf(ff, "%s: %s\n", _("while closing fs"),
+			   error_message(err));
+		ext2fs_free(ff->fs);
+	}
+	ff->fs = NULL;
+
+	if (ff->lockfile)
+		fuse2fs_release_lockfile(ff);
+}
+
 static void op_destroy(void *p EXT2FS_ATTR((unused)))
 {
 	struct fuse2fs *ff = fuse2fs_get();
@@ -5235,16 +5254,7 @@ int main(int argc, char *argv[])
  _("Mount failed while opening filesystem.  Check dmesg(1) for details."));
 		fflush(orig_stderr);
 	}
-	if (fctx.fs) {
-		err = ext2fs_close(fctx.fs);
-		if (err) {
-			com_err(argv[0], err, "while closing fs");
-			ext2fs_free(fctx.fs);
-		}
-		fctx.fs = NULL;
-	}
-	if (fctx.lockfile)
-		fuse2fs_release_lockfile(&fctx);
+	fuse2fs_unmount(&fctx);
 	if (fctx.device)
 		free(fctx.device);
 	fuse_opt_free_args(&args);