[PATCH 0/4] xfs: old lost patches

[PATCH 0/4] xfs: old lost patches

[Eric Sandeen]

4 patches I found in patchworks in various state of review, but no merges.

Resending.

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

[PATCH 1/4] xfs: remove newlines from 3 xfs_alert_tag error strings

[Eric Sandeen]

xfs_alert_tag passes the format string to __xfs_printk,
which adds its own "\n".  Having it in the original string
leads to unintentional blank lines from these messages.

Most format strings have no newline, but these 3 do, leading to
i.e.:

[ 7347.119911] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
[ 7347.119911] 
[ 7347.119919] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
[ 7347.119919] 

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index f47e65c..e6b4202 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1482,7 +1482,7 @@ xfs_bmap_search_extents(
 		xfs_alert_tag(ip->i_mount, XFS_PTAG_FSBLOCK_ZERO,
 				"Access to block zero in inode %llu "
 				"start_block: %llx start_off: %llx "
-				"blkcnt: %llx extent-state: %x lastx: %x\n",
+				"blkcnt: %llx extent-state: %x lastx: %x",
 			(unsigned long long)ip->i_ino,
 			(unsigned long long)gotp->br_startblock,
 			(unsigned long long)gotp->br_startoff,
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 1123d93..40f2985 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -159,7 +159,7 @@ xfs_error_report(
 {
 	if (level <= xfs_error_level) {
 		xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
-		"Internal error %s at line %d of file %s.  Caller 0x%p\n",
+		"Internal error %s at line %d of file %s.  Caller 0x%p",
 			    tag, linenum, filename, ra);
 
 		xfs_stack_trace();
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8d4d49b..43e31b0 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -110,7 +110,7 @@ xfs_alert_fsblock_zero(
 	xfs_alert_tag(ip->i_mount, XFS_PTAG_FSBLOCK_ZERO,
 			"Access to block zero in inode %llu "
 			"start_block: %llx start_off: %llx "
-			"blkcnt: %llx extent-state: %x\n",
+			"blkcnt: %llx extent-state: %x",
 		(unsigned long long)ip->i_ino,
 		(unsigned long long)imap->br_startblock,
 		(unsigned long long)imap->br_startoff,

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

[PATCH 2/4] xfs: reject completely bogus remount options

[Eric Sandeen]

There's a long comment about handling non-remountable
options in xfs_fs_remount, but nothing addresses the case
of completely bogus mount options at remount time, which
can lead to some severe strangeness:

# for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
# for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
# grep sdb4 /etc/mtab
/dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0

This is a bit of a hack, but we can re-use xfs_parseargs()
with a dummy mount struct to just vet all of the remount
options which were passed in.  With this, we get a saner
result:

[44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value

if we try to remount with something ridiculous.

In the long run we should probably revamp a lot of the mount option
handling...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Note, not really sure what KM_* flag would be appropriate here,
if it fails, it really is ok, other than missing the verification.
But maybe that's too "nice?"

commit 72e6ddd901dc8a8ecb835324eb4e11b0d7ad8cf8
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Fri Oct 11 14:03:59 2013 -0500

    xfs: reject completely bogus remount options
    
    There's a long comment about handling non-remountable
    options in xfs_fs_remount, but nothing addresses the case
    of completely bogus mount options at remount time, which
    can lead to some severe strangeness:
    
    # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
    # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
    # grep sdb4 /etc/mtab
    /dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0
    
    This is a bit of a hack, but we can re-use xfs_parseargs()
    with a dummy mount struct to just vet all of the remount
    options which were passed in.  With this, we get a saner
    result:
    
    [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value
    
    if we try to remount with something ridiculous.
    
    In the long run we should probably revamp a lot of the mount option
    handling...
    
    Signed-off-by: Eric Sandeen <sandeen@redhat.com>

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 15188cc..00a06d6 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1202,11 +1202,25 @@ xfs_fs_remount(
 	int			*flags,
 	char			*options)
 {
-	struct xfs_mount	*mp = XFS_M(sb);
+	struct xfs_mount	*mp = XFS_M(sb), *dummy_mp;
 	substring_t		args[MAX_OPT_ARGS];
 	char			*p;
 	int			error;
 
+	/*
+	 * Check all the mount options presented to be sure
+	 * there's nothing too crazy in there.  Non-remountable
+	 * but valid options are a different issue.
+	 */
+	dummy_mp = kmem_zalloc(sizeof(*dummy_mp), KM_MAYFAIL);
+	if (dummy_mp) {
+		dummy_mp->m_super = sb;
+		error = xfs_parseargs(dummy_mp, options);
+		kfree(dummy_mp);
+		if (error)
+			return -error;
+	}
+
 	while ((p = strsep(&options, ",")) != NULL) {
 		int token;
 

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

[PATCH 3/4] xfs: don't emit corruption noise on fs probes

[Eric Sandeen]

If we get EWRONGFS due to probing of non-xfs filesystems,
there's no need to issue the scary corruption error and backtrace.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index a5b59d9..d327b2c 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -624,8 +624,9 @@ xfs_sb_read_verify(
 
 out_error:
 	if (error) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     mp, bp->b_addr);
+		if (error != EWRONGFS)
+			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+					     mp, bp->b_addr);
 		xfs_buf_ioerror(bp, error);
 	}
 }

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

[PATCH 4/4] xfs: don't break from growfs ag update loop on error

[Eric Sandeen]

When xfs_growfs_data_private() is updating backup superblocks,
it bails out on the first error encountered, whether reading or
writing:

* If we get an error writing out the alternate superblocks,
* just issue a warning and continue.  The real work is
* already done and committed.

This can cause a problem later during repair, because repair
looks at all superblocks, and picks the most prevalent one
as correct.  If we bail out early in the backup superblock
loop, we can end up with more "bad" matching superblocks than
good, and a post-growfs repair may revert the filesystem to
the old geometry.

With the combination of superblock verifiers and old bugs,
we're more likely to encounter read errors due to verification.

And perhaps even worse, we don't even properly write any of the
newly-added superblocks in the new AGs.

Even with this change, growfs will still say:

  xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Structure needs cleaning
  data blocks changed from 319815680 to 335216640

which might be confusing to the user, but it at least communicates
that something has gone wrong, and dmesg will probably highlight
the need for an xfs_repair.

And this is still best-effort; if verifiers fail on more than
half the backup supers, they may still "win" - but that's probably
best left to repair to more gracefully handle by doing its own
strict verification as part of the backup super "voting."

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Acked-by: Dave Chinner <david@fromorbit.com>
Reviewed-by: Mark Tinguely <tinguely@sgi.com> 
---

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index e64ee52..634000c 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -153,7 +153,7 @@ xfs_growfs_data_private(
 	xfs_buf_t		*bp;
 	int			bucket;
 	int			dpct;
-	int			error;
+	int			error, saved_error = 0;
 	xfs_agnumber_t		nagcount;
 	xfs_agnumber_t		nagimax = 0;
 	xfs_rfsblock_t		nb, nb_mod;
@@ -496,29 +496,33 @@ xfs_growfs_data_private(
 				error = ENOMEM;
 		}
 
+		/*
+		 * If we get an error reading or writing alternate superblocks,
+		 * continue.  xfs_repair chooses the "best" superblock based
+		 * on most matches; if we break early, we'll leave more
+		 * superblocks un-updated than updated, and xfs_repair may
+		 * pick them over the properly-updated primary.
+		 */
 		if (error) {
 			xfs_warn(mp,
 		"error %d reading secondary superblock for ag %d",
 				error, agno);
-			break;
+			saved_error = error;
+			continue;
 		}
 		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, XFS_SB_ALL_BITS);
 
-		/*
-		 * If we get an error writing out the alternate superblocks,
-		 * just issue a warning and continue.  The real work is
-		 * already done and committed.
-		 */
 		error = xfs_bwrite(bp);
 		xfs_buf_relse(bp);
 		if (error) {
 			xfs_warn(mp,
 		"write error %d updating secondary superblock for ag %d",
 				error, agno);
-			break; /* no point in continuing */
+			saved_error = error;
+			continue;
 		}
 	}
-	return error;
+	return saved_error ? saved_error : error;
 
  error0:
 	xfs_trans_cancel(tp, XFS_TRANS_ABORT);

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

Re: [PATCH 3/4] xfs: don't emit corruption noise on fs probes

[Mark Tinguely]

On 10/11/13 14:12, Eric Sandeen wrote:
> If we get EWRONGFS due to probing of non-xfs filesystems,
> there's no need to issue the scary corruption error and backtrace.
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---

Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH 2/4] xfs: reject completely bogus remount options

[Mark Tinguely]

On 10/11/13 14:11, Eric Sandeen wrote:
> There's a long comment about handling non-remountable
> options in xfs_fs_remount, but nothing addresses the case
> of completely bogus mount options at remount time, which
> can lead to some severe strangeness:
>
> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
> # grep sdb4 /etc/mtab
> /dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0
>
> This is a bit of a hack, but we can re-use xfs_parseargs()
> with a dummy mount struct to just vet all of the remount
> options which were passed in.  With this, we get a saner
> result:
>
> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value
>
> if we try to remount with something ridiculous.
>
> In the long run we should probably revamp a lot of the mount option
> handling...
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---


I don't seem to get the duplicate mtab entries on a top of tree kernel.
Is this still appropriate?

--Mark.

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

Re: [PATCH 1/4] xfs: remove newlines from 3 xfs_alert_tag error strings

[Mark Tinguely]

On 10/11/13 14:09, Eric Sandeen wrote:
> xfs_alert_tag passes the format string to __xfs_printk,
> which adds its own "\n".  Having it in the original string
> leads to unintentional blank lines from these messages.
>
> Most format strings have no newline, but these 3 do, leading to
> i.e.:
>
> [ 7347.119911] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
> [ 7347.119911]
> [ 7347.119919] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
> [ 7347.119919]
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> Reviewed-by: Carlos Maiolino<cmaiolino@redhat.com>
> ---

Is this true of xfs_alert() too? ie the newline in xfs_alert in 
xfs_dir2_leafn_rebalance(). The newline in xfs_alert() in 
xlog_unpack_data_crc() looks intentional.

--Mark.

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

Re: [PATCH 2/4] xfs: reject completely bogus remount options

[Eric Sandeen]

On 10/11/13 4:34 PM, Mark Tinguely wrote:
> On 10/11/13 14:11, Eric Sandeen wrote:
>> There's a long comment about handling non-remountable
>> options in xfs_fs_remount, but nothing addresses the case
>> of completely bogus mount options at remount time, which
>> can lead to some severe strangeness:
>>
>> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
>> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
>> # grep sdb4 /etc/mtab
>> /dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0
>>
>> This is a bit of a hack, but we can re-use xfs_parseargs()
>> with a dummy mount struct to just vet all of the remount
>> options which were passed in.  With this, we get a saner
>> result:
>>
>> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value
>>
>> if we try to remount with something ridiculous.
>>
>> In the long run we should probably revamp a lot of the mount option
>> handling...
>>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> ---
> 
> 
> I don't seem to get the duplicate mtab entries on a top of tree kernel.
> Is this still appropriate?

Maybe different mount(8) behavior on your system?  (probably symlinked to /proc/mounts)

On RHEL6:

# mount /dev/sdb1 /mnt/test
# for I in `seq 1 10`; do mount -o remount,noacl /mnt/test; done 
# mount | grep sdb1
/dev/sdb1 on /mnt/test type xfs (rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl)
# uname -a
Linux hostname 3.12.0-rc4+ #41 SMP Fri Oct 11 19:43:01 CDT 2013 x86_64 x86_64 x86_64 GNU/Linux


-Eric

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

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

Re: [PATCH 1/4] xfs: remove newlines from 3 xfs_alert_tag error strings

[Eric Sandeen]

On 10/11/13 4:59 PM, Mark Tinguely wrote:
> On 10/11/13 14:09, Eric Sandeen wrote:
>> xfs_alert_tag passes the format string to __xfs_printk,
>> which adds its own "\n".  Having it in the original string
>> leads to unintentional blank lines from these messages.
>>
>> Most format strings have no newline, but these 3 do, leading to
>> i.e.:
>>
>> [ 7347.119911] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
>> [ 7347.119911]
>> [ 7347.119919] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
>> [ 7347.119919]
>>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> Reviewed-by: Carlos Maiolino<cmaiolino@redhat.com>
>> ---
> 
> Is this true of xfs_alert() too? ie the newline in xfs_alert in xfs_dir2_leafn_rebalance(). The newline in xfs_alert() in xlog_unpack_data_crc() looks intentional.

All of these:

define_xfs_printk_level(xfs_emerg, KERN_EMERG);
define_xfs_printk_level(xfs_alert, KERN_ALERT);
define_xfs_printk_level(xfs_crit, KERN_CRIT);
define_xfs_printk_level(xfs_err, KERN_ERR);
define_xfs_printk_level(xfs_warn, KERN_WARNING);
define_xfs_printk_level(xfs_notice, KERN_NOTICE);
define_xfs_printk_level(xfs_info, KERN_INFO);
#ifdef DEBUG
define_xfs_printk_level(xfs_debug, KERN_DEBUG);
#endif

go through __xfs_printk(), which adds a newline...

seems like more fixes are in order, yeah.

-Eric
> --Mark.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

[PATCH 1/4 V2] xfs: remove newlines from strings passed to __xfs_printk

[Eric Sandeen]

__xfs_printk adds its own "\n".  Having it in the original string
leads to unintentional blank lines from these messages.

Most format strings have no newline, but a few do, leading to
i.e.:

[ 7347.119911] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
[ 7347.119911] 
[ 7347.119919] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
[ 7347.119919] 

Fix them all.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index f47e65c..e6b4202 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1482,7 +1482,7 @@ xfs_bmap_search_extents(
 		xfs_alert_tag(ip->i_mount, XFS_PTAG_FSBLOCK_ZERO,
 				"Access to block zero in inode %llu "
 				"start_block: %llx start_off: %llx "
-				"blkcnt: %llx extent-state: %x lastx: %x\n",
+				"blkcnt: %llx extent-state: %x lastx: %x",
 			(unsigned long long)ip->i_ino,
 			(unsigned long long)gotp->br_startblock,
 			(unsigned long long)gotp->br_startoff,
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2634700..75e4ea7 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -590,7 +590,7 @@ found:
 		error = _xfs_buf_map_pages(bp, flags);
 		if (unlikely(error)) {
 			xfs_warn(target->bt_mount,
-				"%s: failed to map pages\n", __func__);
+				"%s: failed to map pagesn", __func__);
 			xfs_buf_relse(bp);
 			return NULL;
 		}
@@ -809,7 +809,7 @@ xfs_buf_get_uncached(
 	error = _xfs_buf_map_pages(bp, 0);
 	if (unlikely(error)) {
 		xfs_warn(target->bt_mount,
-			"%s: failed to map pages\n", __func__);
+			"%s: failed to map pages", __func__);
 		goto fail_free_mem;
 	}
 
@@ -1618,7 +1618,7 @@ xfs_setsize_buftarg_flags(
 		bdevname(btp->bt_bdev, name);
 
 		xfs_warn(btp->bt_mount,
-			"Cannot set_blocksize to %u on device %s\n",
+			"Cannot set_blocksize to %u on device %s",
 			sectorsize, name);
 		return EINVAL;
 	}
diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c
index 4c3dba7..34df052 100644
--- a/fs/xfs/xfs_dir2_node.c
+++ b/fs/xfs/xfs_dir2_node.c
@@ -1101,7 +1101,7 @@ xfs_dir2_leafn_rebalance(
 		state->inleaf = 1;
 		blk2->index = 0;
 		xfs_alert(args->dp->i_mount,
-	"%s: picked the wrong leaf? reverting original leaf: blk1->index %d\n",
+	"%s: picked the wrong leaf? reverting original leaf: blk1->index %d",
 			__func__, blk1->index);
 	}
 }
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 1123d93..40f2985 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -159,7 +159,7 @@ xfs_error_report(
 {
 	if (level <= xfs_error_level) {
 		xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
-		"Internal error %s at line %d of file %s.  Caller 0x%p\n",
+		"Internal error %s at line %d of file %s.  Caller 0x%p",
 			    tag, linenum, filename, ra);
 
 		xfs_stack_trace();
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8d4d49b..43e31b0 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -110,7 +110,7 @@ xfs_alert_fsblock_zero(
 	xfs_alert_tag(ip->i_mount, XFS_PTAG_FSBLOCK_ZERO,
 			"Access to block zero in inode %llu "
 			"start_block: %llx start_off: %llx "
-			"blkcnt: %llx extent-state: %x\n",
+			"blkcnt: %llx extent-state: %x",
 		(unsigned long long)ip->i_ino,
 		(unsigned long long)imap->br_startblock,
 		(unsigned long long)imap->br_startoff,
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a2dea10..ea02b48 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1979,7 +1979,7 @@ xlog_print_tic_res(
 
 	for (i = 0; i < ticket->t_res_num; i++) {
 		uint r_type = ticket->t_res_arr[i].r_type;
-		xfs_warn(mp, "region[%u]: %s - %u bytes\n", i,
+		xfs_warn(mp, "region[%u]: %s - %u bytes", i,
 			    ((r_type <= 0 || r_type > XLOG_REG_TYPE_MAX) ?
 			    "bad-rtype" : res_type_str[r_type-1]),
 			    ticket->t_res_arr[i].r_len);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 3979749..e7ca48e 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -305,9 +305,9 @@ xlog_header_check_dump(
 	xfs_mount_t		*mp,
 	xlog_rec_header_t	*head)
 {
-	xfs_debug(mp, "%s:  SB : uuid = %pU, fmt = %d\n",
+	xfs_debug(mp, "%s:  SB : uuid = %pU, fmt = %d",
 		__func__, &mp->m_sb.sb_uuid, XLOG_FMT);
-	xfs_debug(mp, "    log : uuid = %pU, fmt = %d\n",
+	xfs_debug(mp, "    log : uuid = %pU, fmt = %d",
 		&head->h_fs_uuid, be32_to_cpu(head->h_fmt));
 }
 #else
@@ -4077,7 +4077,7 @@ xlog_unpack_data_crc(
 	if (crc != rhead->h_crc) {
 		if (rhead->h_crc || xfs_sb_version_hascrc(&log->l_mp->m_sb)) {
 			xfs_alert(log->l_mp,
-		"log record CRC mismatch: found 0x%x, expected 0x%x.\n",
+		"log record CRC mismatch: found 0x%x, expected 0x%x.",
 					le32_to_cpu(rhead->h_crc),
 					le32_to_cpu(crc));
 			xfs_hex_dump(dp, 32);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 8174aad..fb67091 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -287,7 +287,7 @@ xfs_qm_scall_trunc_qfiles(
 	int		error = 0, error2 = 0;
 
 	if (!xfs_sb_version_hasquota(&mp->m_sb) || flags == 0) {
-		xfs_debug(mp, "%s: flags=%x m_qflags=%x\n",
+		xfs_debug(mp, "%s: flags=%x m_qflags=%x",
 			__func__, flags, mp->m_qflags);
 		return XFS_ERROR(EINVAL);
 	}
@@ -325,7 +325,7 @@ xfs_qm_scall_quotaon(
 	sbflags = 0;
 
 	if (flags == 0) {
-		xfs_debug(mp, "%s: zero flags, m_qflags=%x\n",
+		xfs_debug(mp, "%s: zero flags, m_qflags=%x",
 			__func__, mp->m_qflags);
 		return XFS_ERROR(EINVAL);
 	}
@@ -348,7 +348,7 @@ xfs_qm_scall_quotaon(
 	     (mp->m_sb.sb_qflags & XFS_PQUOTA_ACCT) == 0 &&
 	     (flags & XFS_PQUOTA_ENFD))) {
 		xfs_debug(mp,
-			"%s: Can't enforce without acct, flags=%x sbflags=%x\n",
+			"%s: Can't enforce without acct, flags=%x sbflags=%x",
 			__func__, flags, mp->m_sb.sb_qflags);
 		return XFS_ERROR(EINVAL);
 	}
@@ -648,7 +648,7 @@ xfs_qm_scall_setqlim(
 			q->qi_bsoftlimit = soft;
 		}
 	} else {
-		xfs_debug(mp, "blkhard %Ld < blksoft %Ld\n", hard, soft);
+		xfs_debug(mp, "blkhard %Ld < blksoft %Ld", hard, soft);
 	}
 	hard = (newlim->d_fieldmask & FS_DQ_RTBHARD) ?
 		(xfs_qcnt_t) XFS_BB_TO_FSB(mp, newlim->d_rtb_hardlimit) :
@@ -664,7 +664,7 @@ xfs_qm_scall_setqlim(
 			q->qi_rtbsoftlimit = soft;
 		}
 	} else {
-		xfs_debug(mp, "rtbhard %Ld < rtbsoft %Ld\n", hard, soft);
+		xfs_debug(mp, "rtbhard %Ld < rtbsoft %Ld", hard, soft);
 	}
 
 	hard = (newlim->d_fieldmask & FS_DQ_IHARD) ?
@@ -681,7 +681,7 @@ xfs_qm_scall_setqlim(
 			q->qi_isoftlimit = soft;
 		}
 	} else {
-		xfs_debug(mp, "ihard %Ld < isoft %Ld\n", hard, soft);
+		xfs_debug(mp, "ihard %Ld < isoft %Ld", hard, soft);
 	}
 
 	/*
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index a5b59d9..a89d0bc 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -249,13 +249,13 @@ xfs_mount_validate_sb(
 	if (xfs_sb_version_has_pquotino(sbp)) {
 		if (sbp->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) {
 			xfs_notice(mp,
-			   "Version 5 of Super block has XFS_OQUOTA bits.\n");
+			   "Version 5 of Super block has XFS_OQUOTA bits.");
 			return XFS_ERROR(EFSCORRUPTED);
 		}
 	} else if (sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
 				XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) {
 			xfs_notice(mp,
-"Superblock earlier than Version 5 has XFS_[PQ]UOTA_{ENFD|CHKD} bits.\n");
+"Superblock earlier than Version 5 has XFS_[PQ]UOTA_{ENFD|CHKD} bits.");
 			return XFS_ERROR(EFSCORRUPTED);
 	}
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 15188cc..77093de 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1246,7 +1246,7 @@ xfs_fs_remount(
 			 */
 #if 0
 			xfs_info(mp,
-		"mount option \"%s\" not supported for remount\n", p);
+		"mount option \"%s\" not supported for remount", p);
 			return -EINVAL;
 #else
 			break;

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

Re: [PATCH 1/4 V2] xfs: remove newlines from strings passed to __xfs_printk

[Mark Tinguely]

On 10/11/13 20:59, Eric Sandeen wrote:
> __xfs_printk adds its own "\n".  Having it in the original string
> leads to unintentional blank lines from these messages.
>
> Most format strings have no newline, but a few do, leading to
> i.e.:
>
> [ 7347.119911] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
> [ 7347.119911]
> [ 7347.119919] XFS (sdb2): Access to block zero in inode 132 start_block: 0 start_off: 0 blkcnt: 0 extent-state: 0 lastx: 1a05
> [ 7347.119919]
>
> Fix them all.
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---

Thanks for the updated patch. It looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH 2/4] xfs: reject completely bogus remount options

[Mark Tinguely]

On 10/11/13 20:40, Eric Sandeen wrote:
> On 10/11/13 4:34 PM, Mark Tinguely wrote:
>> On 10/11/13 14:11, Eric Sandeen wrote:
>>> There's a long comment about handling non-remountable
>>> options in xfs_fs_remount, but nothing addresses the case
>>> of completely bogus mount options at remount time, which
>>> can lead to some severe strangeness:
>>>
>>> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
>>> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
>>> # grep sdb4 /etc/mtab
>>> /dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0
>>>
>>> This is a bit of a hack, but we can re-use xfs_parseargs()
>>> with a dummy mount struct to just vet all of the remount
>>> options which were passed in.  With this, we get a saner
>>> result:
>>>
>>> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value
>>>
>>> if we try to remount with something ridiculous.
>>>
>>> In the long run we should probably revamp a lot of the mount option
>>> handling...
>>>
>>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>>> ---
>>
>>
>> I don't seem to get the duplicate mtab entries on a top of tree kernel.
>> Is this still appropriate?
>
> Maybe different mount(8) behavior on your system?  (probably symlinked to /proc/mounts)
>
> On RHEL6:
>
> # mount /dev/sdb1 /mnt/test
> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test; done
> # mount | grep sdb1
> /dev/sdb1 on /mnt/test type xfs (rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl)
> # uname -a
> Linux hostname 3.12.0-rc4+ #41 SMP Fri Oct 11 19:43:01 CDT 2013 x86_64 x86_64 x86_64 GNU/Linux
>
>
> -Eric

Yep, confirmed the described behavior on a RHEL 6 box without patch.
The patch looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH 2/4] xfs: reject completely bogus remount options

[Dave Chinner]

On Fri, Oct 11, 2013 at 02:11:18PM -0500, Eric Sandeen wrote:
> There's a long comment about handling non-remountable
> options in xfs_fs_remount, but nothing addresses the case
> of completely bogus mount options at remount time, which
> can lead to some severe strangeness:
> 
> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
> # grep sdb4 /etc/mtab
> /dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0
> 
> This is a bit of a hack, but we can re-use xfs_parseargs()
> with a dummy mount struct to just vet all of the remount
> options which were passed in.  With this, we get a saner
> result:
> 
> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value

ext4? Really? :)

> +++ b/fs/xfs/xfs_super.c
> @@ -1202,11 +1202,25 @@ xfs_fs_remount(
>  	int			*flags,
>  	char			*options)
>  {
> -	struct xfs_mount	*mp = XFS_M(sb);
> +	struct xfs_mount	*mp = XFS_M(sb), *dummy_mp;
>  	substring_t		args[MAX_OPT_ARGS];
>  	char			*p;
>  	int			error;
>  
> +	/*
> +	 * Check all the mount options presented to be sure
> +	 * there's nothing too crazy in there.  Non-remountable
> +	 * but valid options are a different issue.
> +	 */
> +	dummy_mp = kmem_zalloc(sizeof(*dummy_mp), KM_MAYFAIL);
> +	if (dummy_mp) {
> +		dummy_mp->m_super = sb;
> +		error = xfs_parseargs(dummy_mp, options);
> +		kfree(dummy_mp);
> +		if (error)
> +			return -error;

This, at minimum, leaks dummy_mp->m_fsname, and it will leak other
strings that are also kstrdup()d by xfs_parseargs().

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/4] xfs: reject completely bogus remount options

[Eric Sandeen]

On 10/13/13 4:52 PM, Dave Chinner wrote:
> On Fri, Oct 11, 2013 at 02:11:18PM -0500, Eric Sandeen wrote:
>> There's a long comment about handling non-remountable
>> options in xfs_fs_remount, but nothing addresses the case
>> of completely bogus mount options at remount time, which
>> can lead to some severe strangeness:
>>
>> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
>> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
>> # grep sdb4 /etc/mtab
>> /dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0
>>
>> This is a bit of a hack, but we can re-use xfs_parseargs()
>> with a dummy mount struct to just vet all of the remount
>> options which were passed in.  With this, we get a saner
>> result:
>>
>> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value
> 
> ext4? Really? :)


uhhh ;)

>> +++ b/fs/xfs/xfs_super.c
>> @@ -1202,11 +1202,25 @@ xfs_fs_remount(
>>  	int			*flags,
>>  	char			*options)
>>  {
>> -	struct xfs_mount	*mp = XFS_M(sb);
>> +	struct xfs_mount	*mp = XFS_M(sb), *dummy_mp;
>>  	substring_t		args[MAX_OPT_ARGS];
>>  	char			*p;
>>  	int			error;
>>  
>> +	/*
>> +	 * Check all the mount options presented to be sure
>> +	 * there's nothing too crazy in there.  Non-remountable
>> +	 * but valid options are a different issue.
>> +	 */
>> +	dummy_mp = kmem_zalloc(sizeof(*dummy_mp), KM_MAYFAIL);
>> +	if (dummy_mp) {
>> +		dummy_mp->m_super = sb;
>> +		error = xfs_parseargs(dummy_mp, options);
>> +		kfree(dummy_mp);
>> +		if (error)
>> +			return -error;
> 
> This, at minimum, leaks dummy_mp->m_fsname, and it will leak other
> strings that are also kstrdup()d by xfs_parseargs().

nnngh.  Forgot about that side effect, sorry.  Dammit.

Think it's still worth doing this if I handle freeing them all up?

-Eric

> Cheers,
> 
> Dave.
> 

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

Re: [PATCH 2/4] xfs: reject completely bogus remount options

[Dave Chinner]

On Sun, Oct 13, 2013 at 09:42:37PM -0500, Eric Sandeen wrote:
> On 10/13/13 4:52 PM, Dave Chinner wrote:
> > On Fri, Oct 11, 2013 at 02:11:18PM -0500, Eric Sandeen wrote:
> >> There's a long comment about handling non-remountable
> >> options in xfs_fs_remount, but nothing addresses the case
> >> of completely bogus mount options at remount time, which
> >> can lead to some severe strangeness:
> >>
> >> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
> >> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
> >> # grep sdb4 /etc/mtab
> >> /dev/sdb4 /mnt/test2 xfs rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption 0 0
> >>
> >> This is a bit of a hack, but we can re-use xfs_parseargs()
> >> with a dummy mount struct to just vet all of the remount
> >> options which were passed in.  With this, we get a saner
> >> result:
> >>
> >> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or missing value
> > 
> > ext4? Really? :)
> 
> 
> uhhh ;)
> 
> >> +++ b/fs/xfs/xfs_super.c
> >> @@ -1202,11 +1202,25 @@ xfs_fs_remount(
> >>  	int			*flags,
> >>  	char			*options)
> >>  {
> >> -	struct xfs_mount	*mp = XFS_M(sb);
> >> +	struct xfs_mount	*mp = XFS_M(sb), *dummy_mp;
> >>  	substring_t		args[MAX_OPT_ARGS];
> >>  	char			*p;
> >>  	int			error;
> >>  
> >> +	/*
> >> +	 * Check all the mount options presented to be sure
> >> +	 * there's nothing too crazy in there.  Non-remountable
> >> +	 * but valid options are a different issue.
> >> +	 */
> >> +	dummy_mp = kmem_zalloc(sizeof(*dummy_mp), KM_MAYFAIL);
> >> +	if (dummy_mp) {
> >> +		dummy_mp->m_super = sb;
> >> +		error = xfs_parseargs(dummy_mp, options);
> >> +		kfree(dummy_mp);
> >> +		if (error)
> >> +			return -error;
> > 
> > This, at minimum, leaks dummy_mp->m_fsname, and it will leak other
> > strings that are also kstrdup()d by xfs_parseargs().
> 
> nnngh.  Forgot about that side effect, sorry.  Dammit.
> 
> Think it's still worth doing this if I handle freeing them all up?

If you wrap it all in a helper function (xfs_check-args()?) that
does all the temporary structure allocation and freeing, I think
it will be fine.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/4] xfs: reject completely bogus remount options

[Eric Sandeen]

On 10/13/13 11:45 PM, Dave Chinner wrote:
> On Sun, Oct 13, 2013 at 09:42:37PM -0500, Eric Sandeen wrote:
>> On 10/13/13 4:52 PM, Dave Chinner wrote:
>>> On Fri, Oct 11, 2013 at 02:11:18PM -0500, Eric Sandeen wrote:
>>>> There's a long comment about handling non-remountable
>>>> options in xfs_fs_remount, but nothing addresses the case
>>>> of completely bogus mount options at remount time, which
>>>> can lead to some severe strangeness:
>>>>
>>>> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
>>>> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done

I'm going to just drop this patch for now; it seems too hacky.

really, mount option handling just needs a big rework.

-Eric

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

Re: [PATCH 3/4] xfs: don't emit corruption noise on fs probes

[Christoph Hellwig]

On Fri, Oct 11, 2013 at 02:12:31PM -0500, Eric Sandeen wrote:
> If we get EWRONGFS due to probing of non-xfs filesystems,
> there's no need to issue the scary corruption error and backtrace.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.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 0/4] xfs: old lost patches

[Ben Myers]

On Fri, Oct 11, 2013 at 02:07:54PM -0500, Eric Sandeen wrote:
> 4 patches I found in patchworks in various state of review, but no merges.
> 
> Resending.

Applied patch 1v2, 3, and 4.

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