[PATCH 0/8] xfsprogs: new resvsp db command, plus some fixes

[PATCH 0/8] xfsprogs: new resvsp db command, plus some fixes

[Alex Elder]

The original purpose for this series was to add a new
"resvsp" command to xfs_db, to provide access to the
xfsctl(3) XFS_IOC_RESVSP64 operation.  Kevan Rehm did
this work, and along the way he found and fixed a few
cases where things weren't getting flushed to disk
properly.  I also looked things over and came up with
a few other small fixes.

I'm submitting the resulting series on Kevan's behalf.
I think we'll want to follow this up with at least one
test in the xfstests suite to exercise this new command.

					-Alex

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/8] xfsprogs: Fix setbitval() bug when nbits is byte-aligned

[Alex Elder]

From: Kevan Rehm <kfr@sgi.com>

If a field whose size is not an even multiple of 8 bits happens to
be aligned on a byte boundary and the machine is little-endian,
routine setbitval() would do a byte copy of zero bytes, effectively
doing nothing.

Catch this case, and arrange for doing the bit setting "the hard
way" (i.e., without using memcpy()) to avoid this problem.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 db/bit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/db/bit.c b/db/bit.c
index ca57d31..36de0a1 100644
--- a/db/bit.c
+++ b/db/bit.c
@@ -153,7 +153,7 @@ setbitval(
 		 */
 
 		/* byte aligned ? */
-		if (bitoff%NBBY) {
+		if (bitoff % NBBY || nbits % NBBY) {
 			/* no - bit copy */
 			for (bit=0; bit<nbits; bit++)
 				setbit(out, bit+bitoff, getbit(in, bit));
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/8] xfsprogs: unconditionally drop used buffer reference

[Alex Elder]

In libxfs_mount(), after reading the last block in the log device,
the buffer is released by a call to libxfs_putbuf().  But it's done
only if bp is non-null.  It always will be non-null at this point,
so just make the call unconditionally.

Also touch up a misleading indent.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxfs/init.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/libxfs/init.c b/libxfs/init.c
index 08fc584..ba44c9b 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -743,15 +743,14 @@ libxfs_mount(
 			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
 				return NULL;
 		}
-		if (bp)
-			libxfs_putbuf(bp);
+		libxfs_putbuf(bp);
 	}
 
 	/* Initialize realtime fields in the mount structure */
 	if (rtmount_init(mp, flags)) {
 		fprintf(stderr, _("%s: realtime device init failed\n"),
 			progname);
-			return NULL;
+		return NULL;
 	}
 
 	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/8] xfsprogs: xfs_repair: don't set the root inode pointer

[Alex Elder]

From: Kevan Rehm <kfr@sgi.com>

In phase 6, in mk_root_dir(), xfs_repair initializes the mount
point's m_rootip pointer without accounting for that reference.
This field never really used or needed in repair otherwise, and the
assigned pointer doesn't really represent a real reference to an
inode that needs to be cached.  So just kill off this assignment.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 repair/phase6.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index 1c82cb1..81d7fe6 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -797,8 +797,6 @@ mk_root_dir(xfs_mount_t *mp)
 	ip->i_df.if_bytes = ip->i_df.if_real_bytes = 0;
 	ip->i_df.if_u1.if_extents = NULL;
 
-	mp->m_rootip = ip;
-
 	/*
 	 * initialize the directory
 	 */
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/8] xfsprogs: mkfs.xfs: let libxfs_umount() do its thing

[Alex Elder]

From: Kevan Rehm <kfr@sgi.com>

In the mkfs.xfs main() routine (which is gargantuan, by the way),
there are a few function calls intended to do some clean up before
unmounting the filesystem.

The first function drops references that will be dropped again in
the unmnount call.  The others assume all references will go away as
a result of the purge, but the filesystem's root inode reference
will remain.

Let the unmount function take care of destroying the real-time
structures.  And instead of purging the inode and block caches, just
flush them (at least the block cache) and let the unmount code take
care of the final purge.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 mkfs/xfs_mkfs.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f527f3d..213cc74 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2674,9 +2674,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
 	 * Dump all inodes and buffers before marking us all done.
 	 * Need to drop references to inodes we still hold, first.
 	 */
-	libxfs_rtmount_destroy(mp);
-	libxfs_icache_purge();
-	libxfs_bcache_purge();
+	libxfs_bcache_flush();
 
 	/*
 	 * Mark the filesystem ok.
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/8] xfsprogs: Drop root inode refrerence in libxfs_umount()

[Alex Elder]

From: Kevan Rehm <kfr@sgi.com>

Routine libxfs_umount() did not call libxfs_iput for the m_rootip
inode, so updates made to that inode could be lost.  This adds the
missing call, and re-initializes the m_rootip pointer to be null.

Since the root inode reference is now dropped by libxfs_umount(), it
should *not* be dropped in mkfs parseproto().

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxfs/init.c |    4 ++++
 mkfs/proto.c  |    3 ++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/libxfs/init.c b/libxfs/init.c
index ba44c9b..d34fd8c 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -816,6 +816,10 @@ libxfs_umount(xfs_mount_t *mp)
 	int			agno;
 
 	libxfs_rtmount_destroy(mp);
+	if (mp->m_rootip) {
+		libxfs_iput(mp->m_rootip, 0);
+		mp->m_rootip = NULL;
+	}
 	libxfs_icache_purge();
 	libxfs_bcache_purge();
 
diff --git a/mkfs/proto.c b/mkfs/proto.c
index 3021028..3f76862 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -571,7 +571,8 @@ parseproto(
 				break;
 			parseproto(mp, ip, fsxp, pp, name);
 		}
-		libxfs_iput(ip, 0);
+		if (!isroot)
+			libxfs_iput(ip, 0);
 		return;
 	}
 	libxfs_trans_log_inode(tp, ip, flags);
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 6/8] xfsprogs: xfs_db: unmount fs before exiting

[Alex Elder]

From: Kevan Rehm <kfr@sgi.com>

The main routine calls libxfs_mount() via init(), but never calls
libxfs_umount() at the end of the program so there could be
filesystem changes queued which never get flushed to disk.  This
adds the missing libxfs_umount() call.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 db/init.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/db/init.c b/db/init.c
index 2a5ef2b..5effaf9 100644
--- a/db/init.c
+++ b/db/init.c
@@ -170,7 +170,7 @@ main(
 	}
 	if (cmdline) {
 		xfree(cmdline);
-		return exitcode;
+		goto end;
 	}
 
 	while (!done) {
@@ -181,5 +181,9 @@ main(
 			done = command(c, v);
 		doneline(input, v);
 	}
+
+end:
+	libxfs_umount(mp);
+
 	return exitcode;
 }
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 7/8] xfsprogs: clean up errors in libxfs_mount() consistently

[Alex Elder]

Until the perag structures for the filesystem have been set up,
initialization of the mount point only assigns computed values and
has no other side-effects (such as allocating additional
structures).  So up to that point, returning a null pointer to
signal an error is adequate.  Once the perag initialization is done
there needs to be some teardown in case of an error.

Here the handling of errors is inconsistent.  If early perag
initialization fails, the code currently just exits.  Then, if
getting a reference to the root inode results in an error, a null
pointer is returned but without first cleaning up the perag
structures.  Next, if rtmount_inodes() returns an error, the
reference to the root inode (if any) is released, but again the
perag structures are not cleaned up.  Finally, when the perag data
is read in, if an error occurs, a null pointer is returned but the
root inode pointer reference is not released and the perag
structures are not cleaned up.

Remedy all of that by having each of these error cases jump to error
handling code at the end of the function.  That code needs to
release the reference to the root inode and release all of the perag
structures.  This (plus a few other things that will be no-ops at
this point in the mount process) is exactly what libxfs_umount(), so
just use that function to implement this cleanup activity.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxfs/init.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/libxfs/init.c b/libxfs/init.c
index d34fd8c..9dad5b7 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -757,7 +757,7 @@ libxfs_mount(
 	if (error) {
 		fprintf(stderr, _("%s: perag init failed\n"),
 			progname);
-		exit(1);
+		goto out_err;
 	}
 
 	/*
@@ -770,15 +770,12 @@ libxfs_mount(
 			fprintf(stderr, _("%s: cannot read root inode (%d)\n"),
 				progname, error);
 			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
-				return NULL;
+				goto out_err;
 		}
 		ASSERT(mp->m_rootip != NULL);
 	}
-	if ((flags & LIBXFS_MOUNT_ROOTINOS) && rtmount_inodes(mp)) {
-		if (mp->m_rootip)
-			libxfs_iput(mp->m_rootip, 0);
-		return NULL;
-	}
+	if ((flags & LIBXFS_MOUNT_ROOTINOS) && rtmount_inodes(mp))
+		goto out_err;
 
 	/*
 	 * mkfs calls mount before the AGF/AGI structures are written.
@@ -789,11 +786,16 @@ libxfs_mount(
 		if (error) {
 			fprintf(stderr, _("%s: cannot init perag data (%d)\n"),
 				progname, error);
-			return NULL;
+			goto out_err;
 		}
 	}
 
 	return mp;
+
+out_err:
+	libxfs_umount(mp);	/* clean up first */
+
+	return NULL;
 }
 
 void
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 8/8] xfsprogs: xfs_db: add new "resvsp" command

[Alex Elder]

From: Kevan Rehm <kfr@sgi.com>

This patch adds a new "resvsp" command to xfs_db.  The command
provides access to the xfsctl(3) XFS_IOC_RESVSP64 operation, which
allocates space in an ordinary file.  Blocks are allocated but not
zeroed, and the file size does not change.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 db/Makefile  |    2 +-
 db/command.c |    2 +
 db/resvsp.c  |  184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 db/resvsp.h  |   19 ++++++
 4 files changed, 206 insertions(+), 1 deletions(-)
 create mode 100644 db/resvsp.c
 create mode 100644 db/resvsp.h

diff --git a/db/Makefile b/db/Makefile
index 5c7d054..797ff52 100644
--- a/db/Makefile
+++ b/db/Makefile
@@ -12,7 +12,7 @@ HFILES = addr.h agf.h agfl.h agi.h attr.h attrshort.h bit.h block.h bmap.h \
 	dir.h dir2.h dir2sf.h dirshort.h dquot.h echo.h faddr.h field.h \
 	flist.h fprint.h frag.h freesp.h hash.h help.h init.h inode.h input.h \
 	io.h malloc.h metadump.h output.h print.h quit.h sb.h sig.h strvec.h \
-	text.h type.h write.h attrset.h
+	text.h type.h write.h attrset.h resvsp.h
 CFILES = $(HFILES:.h=.c)
 LSRCFILES = xfs_admin.sh xfs_check.sh xfs_ncheck.sh xfs_metadump.sh
 
diff --git a/db/command.c b/db/command.c
index b7e3165..b70e1ef 100644
--- a/db/command.c
+++ b/db/command.c
@@ -43,6 +43,7 @@
 #include "metadump.h"
 #include "output.h"
 #include "print.h"
+#include "resvsp.h"
 #include "quit.h"
 #include "sb.h"
 #include "write.h"
@@ -121,6 +122,7 @@ init_commands(void)
 	attrset_init();
 	block_init();
 	bmap_init();
+	resvsp_init();
 	check_init();
 	convert_init();
 	debug_init();
diff --git a/db/resvsp.c b/db/resvsp.c
new file mode 100644
index 0000000..c7206e2
--- /dev/null
+++ b/db/resvsp.c
@@ -0,0 +1,184 @@
+
+/*
+ * Copyright (c) 2011 SGI
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include <xfs/libxfs.h>
+#include "command.h"
+#include "type.h"
+#include "fprint.h"
+#include "faddr.h"
+#include "field.h"
+#include "bmap.h"
+#include "io.h"
+#include "inode.h"
+#include "output.h"
+#include "init.h"
+#include "resvsp.h"
+
+static int	resvsp_f(int argc, char **argv);
+static void	resvsp_help(void);
+
+static const cmdinfo_t	resvsp_cmd =
+	{ "resvsp", NULL, resvsp_f, 0, -1, 0,
+	N_("[-w] [-o offset] [-l length] [inode#]"),
+	N_("allocate space in a file"), resvsp_help };
+
+static void
+resvsp_help(void)
+{
+	dbprintf(_(
+"\n"
+"The resvsp function is essentially an implementation of the xfsctl(3)\n"
+"file operation XFS_IOC_RESVSP64 which allocates space in an ordinary\n"
+"file.  Blocks are allocated but not zeroed, and the file size does not\n"
+"change.  The -o option is the starting offset for the allocation (default 0)\n"
+"and the -l option gives the length of the allocation in bytes (default to\n"
+"end of file).  Both offset and length values will be rounded to a filesystem\n"
+"block boundary.  'inode#' is the inode number of the file in which to\n"
+"perform the allocation.  If none is specified, the current inode is used.\n"
+"If the -w option is specified, the allocated extents will not be flagged as\n"
+"unwritten.  Use this option with care, as someone with read permission\n"
+"to the file can then read whatever is written in those blocks.\n"
+"\n"
+" Example:\n"
+"\n"
+" 'resvsp'                  - allocate blocks in the current inode's entire byte range\n"
+" 'resvsp 76'               - allocate blocks in the entire byte range of inode 76\n"
+" 'resvsp -o 8192 -l 4096'  - allocate 4096 bytes at offset 8192 in the current inode\n"
+"\n"
+));
+}
+
+static int
+resvsp_f(
+	int		argc,
+	char		**argv)
+{
+	xfs_inode_t	*ip;
+	int		error;
+	int		c;
+	char		*p;
+	xfs_ino_t	ino;
+	xfs_off_t	offset = 0;	/* allocate from start of file */
+	xfs_off_t	len = 0;	/* allocate to EOF */
+	int		alloc_type = 1;	/* flag extents as unwritten */
+
+	if (x.isreadonly & LIBXFS_ISREADONLY) {
+		dbprintf(_("%s started in read only mode, resvsp disabled\n"),
+			progname);
+		return 0;
+	}
+
+	optind = 0;
+	while ((c = getopt(argc, argv, "l:o:w")) != EOF) {
+		switch (c) {
+		case 'l':
+			len = strtoull(optarg, &p, 0);
+			if (*p != '\0') {
+				dbprintf(_("bad length %s specified for "
+					"-l option\n"), optarg);
+				return 0;
+			}
+			break;
+
+		case 'o':
+			offset = strtoull(optarg, &p, 0);
+			if (*p != '\0') {
+				dbprintf(_("bad offset %s specified for "
+					"-o option\n"), optarg);
+				return 0;
+			}
+			break;
+
+		case 'w':
+			alloc_type = 0;	/* do not flag extents as unwritten */
+			break;
+
+		default:
+			/* getopt() provides the error message */
+			return 0;
+		}
+	}
+	if (optind < argc) {
+		ino = strtoull(argv[optind], &p, 0);
+		if (*p != '\0') {
+			dbprintf(_("bad value %s specified for inode number\n"),
+				argv[optind]);
+			return 0;
+		}
+		optind++;
+	} else if (iocur_top->ino != NULLFSINO) {
+		ino = iocur_top->ino;
+	} else {
+		dbprintf(_("no current inode, and no inode number provided\n"));
+		return 0;
+	}
+	if (optind < argc) {
+		dbprintf(_("only one inode number allowed\n"));
+		return 0;
+	}
+
+	if (libxfs_iget(mp, NULL, ino, 0, &ip, 0)) {
+		dbprintf(_("failed to iget inode %llu\n"), ino);
+		return 0;
+	}
+
+	if ((ip->i_d.di_mode & S_IFMT) != S_IFREG) {
+		dbprintf(_("inode %llu is not an ordinary file\n"), ino);
+		goto fail;
+	}
+
+	if (offset >= ip->i_size) {
+		dbprintf(_("offset %llu must be <= file length %llu\n"),
+			offset, ip->i_size);
+		goto fail;
+	}
+	if (offset + len > ip->i_size) {
+		len = ip->i_size - offset;
+		dbprintf(_("length trimmed to %llu bytes\n"), len);
+	}
+	if (len == 0) {
+		len = ip->i_size - offset;
+	}
+
+	error = libxfs_alloc_file_space(ip, offset, len, alloc_type, 0);
+
+	if (error) {
+		dbprintf(_("error reserving space for a file, %d\n"), error);
+		return 0;
+	}
+
+	/* refresh with updated inode contents */
+	if (iocur_top->ino == ino) {
+		set_cur_inode(iocur_top->ino);
+	}
+
+fail:
+	libxfs_iput(ip, 0);
+
+	return 0;
+}
+
+void
+resvsp_init(void)
+{
+	if (!expert_mode)
+		return;
+
+	add_command(&resvsp_cmd);
+}
diff --git a/db/resvsp.h b/db/resvsp.h
new file mode 100644
index 0000000..b98da8b
--- /dev/null
+++ b/db/resvsp.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2011 SGI
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+extern void	resvsp_init(void);
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 8/8] xfsprogs: xfs_db: add new "resvsp" command

[Dave Chinner]

On Thu, Nov 10, 2011 at 02:35:18PM -0600, Alex Elder wrote:
> From: Kevan Rehm <kfr@sgi.com>
> 
> This patch adds a new "resvsp" command to xfs_db.  The command
> provides access to the xfsctl(3) XFS_IOC_RESVSP64 operation, which
> allocates space in an ordinary file.  Blocks are allocated but not
> zeroed, and the file size does not change.

The commit message fails to answer the most important question of
all: what is the use case that requires adding offline preallocation
of extents to arbitrary inodes?

Conceptually, I don't think high level functionality like extent
allocation belongs in xfs_db:

	- extent allocation is effectively an open-ended, multi-step
	  modification that can touch large amounts of metadata
	  across all AGs in the filesystem,

	- userspace extent allocation code is rarely used and as
	  such nowhere near as well tested as the kernel code.

	- userspace extent allocation does not support real time
	  device allocation at all:

	  #define xfs_bmap_rtalloc(a)                     (ENOSYS)

	- modifications made by xfs_db are not transactional and
	  therefore not recoverable if the system dies during such
	  an operation. Such a failure means you have to run a
	  xfs_reapir pass on your filesystem to clean up the mess
	  that was left behind....

	- we already have tools to do space reservation online
	  (i.e. xfs_io -c "resvsp off len" testfile) in a safe
	  manner that also supports realtime device allocation. Why
	  duplicate this functionality in a different tool?


.....

> +static void
> +resvsp_help(void)
> +{
> +	dbprintf(_(
> +"\n"
> +"The resvsp function is essentially an implementation of the xfsctl(3)\n"
> +"file operation XFS_IOC_RESVSP64 which allocates space in an ordinary\n"
> +"file.

Probably no need to mention the ioctl here - preallocation is a well
enough known function now that simply calling it "an extent
preallocation operation" is sufficient to explain what it is for.

> Blocks are allocated but not zeroed, and the file size does not\n"
> +"change.  The -o option is the starting offset for the allocation (default 0)\n"
> +"and the -l option gives the length of the allocation in bytes (default to\n"
> +"end of file).  Both offset and length values will be rounded to a filesystem\n"
> +"block boundary.  'inode#' is the inode number of the file in which to\n"
> +"perform the allocation.  If none is specified, the current inode is used.\n"
> +"If the -w option is specified, the allocated extents will not be flagged as\n"
> +"unwritten.  Use this option with care, as someone with read permission\n"
> +"to the file can then read whatever is written in those blocks.\n"

I'm on record as being totally against making it easy for -anyone-
to preallocate _written_ extents via -any method- because it's just
a whopping great big security hole. "Use care" is not a good enough
protection against the potential of exposing large amount of stale
data to any user that can read the file, even though only root can
do that preallocation.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/8] xfsprogs: unconditionally drop used buffer reference

[Christoph Hellwig]

On Thu, Nov 10, 2011 at 02:35:12PM -0600, Alex Elder wrote:
> In libxfs_mount(), after reading the last block in the log device,
> the buffer is released by a call to libxfs_putbuf().  But it's done
> only if bp is non-null.  It always will be non-null at this point,
> so just make the call unconditionally.
> 
> Also touch up a misleading indent.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>
> ---
>  libxfs/init.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 08fc584..ba44c9b 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -743,15 +743,14 @@ libxfs_mount(
>  			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
>  				return NULL;
>  		}
> -		if (bp)
> -			libxfs_putbuf(bp);
> +		libxfs_putbuf(bp);
>  	}

This one isn't correct.  If LIBXFS_MOUNT_DEBUGGER is set we still
get through to here.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/8] xfsprogs: xfs_repair: don't set the root inode pointer

[Christoph Hellwig]

On Thu, Nov 10, 2011 at 02:35:13PM -0600, Alex Elder wrote:
> From: Kevan Rehm <kfr@sgi.com>
> 
> In phase 6, in mk_root_dir(), xfs_repair initializes the mount
> point's m_rootip pointer without accounting for that reference.
> This field never really used or needed in repair otherwise, and the
> assigned pointer doesn't really represent a real reference to an
> inode that needs to be cached.  So just kill off this assignment.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>

Shouldn't this also have Kevan's signoff?

> ---
>  repair/phase6.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 1c82cb1..81d7fe6 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -797,8 +797,6 @@ mk_root_dir(xfs_mount_t *mp)
>  	ip->i_df.if_bytes = ip->i_df.if_real_bytes = 0;
>  	ip->i_df.if_u1.if_extents = NULL;
>  
> -	mp->m_rootip = ip;
> -

Looks good, but can we extent this a bit?  The only reference to
mp->m_rootip in the userspace code is in libxfs_mount.  By making
it a local variable there we can kill the field entirely.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/8] xfsprogs: mkfs.xfs: let libxfs_umount() do its thing

[Christoph Hellwig]

On Thu, Nov 10, 2011 at 02:35:14PM -0600, Alex Elder wrote:
> From: Kevan Rehm <kfr@sgi.com>
> 
> In the mkfs.xfs main() routine (which is gargantuan, by the way),
> there are a few function calls intended to do some clean up before
> unmounting the filesystem.
> 
> The first function drops references that will be dropped again in
> the unmnount call.  The others assume all references will go away as
> a result of the purge, but the filesystem's root inode reference
> will remain.
> 
> Let the unmount function take care of destroying the real-time
> structures.  And instead of purging the inode and block caches, just
> flush them (at least the block cache) and let the unmount code take
> care of the final purge.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>
> ---
>  mkfs/xfs_mkfs.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f527f3d..213cc74 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2674,9 +2674,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
>  	 * Dump all inodes and buffers before marking us all done.
>  	 * Need to drop references to inodes we still hold, first.
>  	 */
> -	libxfs_rtmount_destroy(mp);

After his libxfs_rtmount_destroy should be marked static.
> -	libxfs_icache_purge();
> -	libxfs_bcache_purge();
> +	libxfs_bcache_flush();

Why don't we need a flush for the inode cache here?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/8] xfsprogs: Drop root inode refrerence in libxfs_umount()

[Christoph Hellwig]

On Thu, Nov 10, 2011 at 02:35:15PM -0600, Alex Elder wrote:
> From: Kevan Rehm <kfr@sgi.com>
> 
> Routine libxfs_umount() did not call libxfs_iput for the m_rootip
> inode, so updates made to that inode could be lost.  This adds the
> missing call, and re-initializes the m_rootip pointer to be null.
> 
> Since the root inode reference is now dropped by libxfs_umount(), it
> should *not* be dropped in mkfs parseproto().

Looks fine in general, but as mentioned before live would be a lot
easier if we simply made the root inode a local variable in
libxfs_mount.

In fact I wonder if we even want to do that, there seems very little
reason to even do the root inode iget there - the only caller requesting
it will fall back if it fails.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 6/8] xfsprogs: xfs_db: unmount fs before exiting

[Christoph Hellwig]

On Thu, Nov 10, 2011 at 02:35:16PM -0600, Alex Elder wrote:
> From: Kevan Rehm <kfr@sgi.com>
> 
> The main routine calls libxfs_mount() via init(), but never calls
> libxfs_umount() at the end of the program so there could be
> filesystem changes queued which never get flushed to disk.  This
> adds the missing libxfs_umount() call.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 7/8] xfsprogs: clean up errors in libxfs_mount() consistently

[Christoph Hellwig]

On Thu, Nov 10, 2011 at 02:35:17PM -0600, Alex Elder wrote:
> Until the perag structures for the filesystem have been set up,
> initialization of the mount point only assigns computed values and
> has no other side-effects (such as allocating additional
> structures).  So up to that point, returning a null pointer to
> signal an error is adequate.  Once the perag initialization is done
> there needs to be some teardown in case of an error.
> 
> Here the handling of errors is inconsistent.  If early perag
> initialization fails, the code currently just exits.  Then, if
> getting a reference to the root inode results in an error, a null
> pointer is returned but without first cleaning up the perag
> structures.  Next, if rtmount_inodes() returns an error, the
> reference to the root inode (if any) is released, but again the
> perag structures are not cleaned up.  Finally, when the perag data
> is read in, if an error occurs, a null pointer is returned but the
> root inode pointer reference is not released and the perag
> structures are not cleaned up.
> 
> Remedy all of that by having each of these error cases jump to error
> handling code at the end of the function.  That code needs to
> release the reference to the root inode and release all of the perag
> structures.  This (plus a few other things that will be no-ops at
> this point in the mount process) is exactly what libxfs_umount(), so
> just use that function to implement this cleanup activity.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>

Did you make sure libxfs_umount doesn't do too much work?  I'm usually
more of a fan of uninding each operation individually.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/8] xfsprogs: xfs_repair: don't set the root inode pointer

[Christoph Hellwig]

On Sun, Nov 13, 2011 at 07:01:33AM -0500, Christoph Hellwig wrote:
> On Thu, Nov 10, 2011 at 02:35:13PM -0600, Alex Elder wrote:
> > From: Kevan Rehm <kfr@sgi.com>
> > 
> > In phase 6, in mk_root_dir(), xfs_repair initializes the mount
> > point's m_rootip pointer without accounting for that reference.
> > This field never really used or needed in repair otherwise, and the
> > assigned pointer doesn't really represent a real reference to an
> > inode that needs to be cached.  So just kill off this assignment.
> > 
> > Signed-off-by: Alex Elder <aelder@sgi.com>
> 
> Shouldn't this also have Kevan's signoff?

Kevan, can you give me a signoff for this patch, I'd like to put it in?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/8] xfsprogs: xfs_repair: don't set the root inode pointer

[Kevan Rehm]

On 02/07/2012 12:38 PM, Christoph Hellwig wrote:
> On Sun, Nov 13, 2011 at 07:01:33AM -0500, Christoph Hellwig wrote:
>> On Thu, Nov 10, 2011 at 02:35:13PM -0600, Alex Elder wrote:
>>> From: Kevan Rehm<kfr@sgi.com>
>>>
>>> In phase 6, in mk_root_dir(), xfs_repair initializes the mount
>>> point's m_rootip pointer without accounting for that reference.
>>> This field never really used or needed in repair otherwise, and the
>>> assigned pointer doesn't really represent a real reference to an
>>> inode that needs to be cached.  So just kill off this assignment.
>>>
>>> Signed-off-by: Alex Elder<aelder@sgi.com>
>> Shouldn't this also have Kevan's signoff?
> Kevan, can you give me a signoff for this patch, I'd like to put it in?
>
Fine by me.

Kevan

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/8] xfsprogs: xfs_repair: don't set the root inode pointer

[Kevan Rehm]

Signed-off-by: Kevan Rehm<kfr@sgi.com>

I guess this is the proper way, sorry.

Kevan

On 02/07/2012 02:25 PM, Kevan Rehm wrote:
> On 02/07/2012 12:38 PM, Christoph Hellwig wrote:
>> On Sun, Nov 13, 2011 at 07:01:33AM -0500, Christoph Hellwig wrote:
>>> On Thu, Nov 10, 2011 at 02:35:13PM -0600, Alex Elder wrote:
>>>> From: Kevan Rehm<kfr@sgi.com>
>>>>
>>>> In phase 6, in mk_root_dir(), xfs_repair initializes the mount
>>>> point's m_rootip pointer without accounting for that reference.
>>>> This field never really used or needed in repair otherwise, and the
>>>> assigned pointer doesn't really represent a real reference to an
>>>> inode that needs to be cached.  So just kill off this assignment.
>>>>
>>>> Signed-off-by: Alex Elder<aelder@sgi.com>
>>> Shouldn't this also have Kevan's signoff?
>> Kevan, can you give me a signoff for this patch, I'd like to put it in?
>>
> Fine by me.
>
> Kevan

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs