[PATCH 0/3] jfs: logging neatening

[PATCH 0/3] jfs: logging neatening

[Joe Perches]

This patchset fixes the uses of jfs_info, jfs_warn and jfs_err that
have terminating newlines and a couple other trivialities to make
the logging a bit more consistent.

There is a difference in use between jfs_error and the other
jfs_info, jfs_warn, and jfs_err logging macros.  jfs_error is more
like the rest of the kernel and requires a newline as the last
character of the format.

The jfs_info, jfs_warn, and jfs_err macros add the terminating
newline to the format so the uses do not require them.

It is a habituated style for many people to add the terminating
newline for many people and this difference in use between the
various macro styles causes trivial defects in logging output.

It might be better if the jfs_info, jfs_warn, and jfs_err macros
were changed to require a newline termination and the uses changed
to include the newline, but that's a larger change.

Joe Perches (3):
  jfs: Remove terminating newlines from jfs_info, jfs_warn, jfs_err uses
  jfs: Remove unnecessary line continuations and terminating newlines
  jfs: Coalesce some formats

 fs/jfs/inode.c       |  4 ++--
 fs/jfs/jfs_discard.c |  6 ++----
 fs/jfs/jfs_dtree.c   | 10 ++++------
 fs/jfs/jfs_imap.c    |  3 +--
 fs/jfs/jfs_inode.c   |  2 +-
 fs/jfs/jfs_logmgr.c  | 14 ++++++--------
 fs/jfs/jfs_txnmgr.c  | 21 +++++++++------------
 fs/jfs/namei.c       |  4 ++--
 fs/jfs/super.c       |  4 ++--
 9 files changed, 29 insertions(+), 39 deletions(-)

-- 
2.8.0.rc4.16.g56331f8

[PATCH 1/3] jfs: Remove terminating newlines from jfs_info, jfs_warn, jfs_err uses

[Joe Perches]

These macros add the newline so these cause extra blank lines
in logging output.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/jfs/jfs_inode.c  | 2 +-
 fs/jfs/jfs_logmgr.c | 4 ++--
 fs/jfs/jfs_txnmgr.c | 4 ++--
 fs/jfs/super.c      | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/jfs/jfs_inode.c b/fs/jfs/jfs_inode.c
index cf7936f..5e33cb9 100644
--- a/fs/jfs/jfs_inode.c
+++ b/fs/jfs/jfs_inode.c
@@ -151,7 +151,7 @@ struct inode *ialloc(struct inode *parent, umode_t mode)
 	jfs_inode->xtlid = 0;
 	jfs_set_inode_flags(inode);
 
-	jfs_info("ialloc returns inode = 0x%p\n", inode);
+	jfs_info("ialloc returns inode = 0x%p", inode);
 
 	return inode;
 
diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index a270cb7f..7638355 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -1094,7 +1094,7 @@ int lmLogOpen(struct super_block *sb)
 		if (log->bdev->bd_dev == sbi->logdev) {
 			if (memcmp(log->uuid, sbi->loguuid,
 				   sizeof(log->uuid))) {
-				jfs_warn("wrong uuid on JFS journal\n");
+				jfs_warn("wrong uuid on JFS journal");
 				mutex_unlock(&jfs_log_mutex);
 				return -EINVAL;
 			}
@@ -2136,7 +2136,7 @@ static void lbmStartIO(struct lbuf * bp)
 	struct bio *bio;
 	struct jfs_log *log = bp->l_log;
 
-	jfs_info("lbmStartIO\n");
+	jfs_info("lbmStartIO");
 
 	bio = bio_alloc(GFP_NOFS, 1);
 	bio->bi_iter.bi_sector = bp->l_blkno << (log->l2bsize - 9);
diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
index d595856..51421a8 100644
--- a/fs/jfs/jfs_txnmgr.c
+++ b/fs/jfs/jfs_txnmgr.c
@@ -1764,7 +1764,7 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,
 		if (lwm == next)
 			goto out;
 		if (lwm > next) {
-			jfs_err("xtLog: lwm > next\n");
+			jfs_err("xtLog: lwm > next");
 			goto out;
 		}
 		tlck->flag |= tlckUPDATEMAP;
@@ -2814,7 +2814,7 @@ int jfs_lazycommit(void *arg)
 	if (!list_empty(&TxAnchor.unlock_queue))
 		jfs_err("jfs_lazycommit being killed w/pending transactions!");
 	else
-		jfs_info("jfs_lazycommit being killed\n");
+		jfs_info("jfs_lazycommit being killed");
 	return 0;
 }
 
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 4f5d85b..f908012 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -84,7 +84,7 @@ static void jfs_handle_error(struct super_block *sb)
 		panic("JFS (device %s): panic forced after error\n",
 			sb->s_id);
 	else if (sbi->flag & JFS_ERR_REMOUNT_RO) {
-		jfs_err("ERROR: (device %s): remounting filesystem as read-only\n",
+		jfs_err("ERROR: (device %s): remounting filesystem as read-only",
 			sb->s_id);
 		sb->s_flags |= MS_RDONLY;
 	}
@@ -641,7 +641,7 @@ static int jfs_freeze(struct super_block *sb)
 		}
 		rc = updateSuper(sb, FM_CLEAN);
 		if (rc) {
-			jfs_err("jfs_freeze: updateSuper failed\n");
+			jfs_err("jfs_freeze: updateSuper failed");
 			/*
 			 * Don't fail here. Everything succeeded except
 			 * marking the superblock clean, so there's really
-- 
2.8.0.rc4.16.g56331f8

[PATCH 2/3] jfs: Remove unnecessary line continuations and terminating newlines

[Joe Perches]

These jfs_<level> uses need neither a line continuation to assemble
the format strings nor newline terminations in the formats.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/jfs/jfs_discard.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/jfs/jfs_discard.c b/fs/jfs/jfs_discard.c
index dfcd503..f76ff0a 100644
--- a/fs/jfs/jfs_discard.c
+++ b/fs/jfs/jfs_discard.c
@@ -49,14 +49,12 @@ void jfs_issue_discard(struct inode *ip, u64 blkno, u64 nblocks)
 
 	r = sb_issue_discard(sb, blkno, nblocks, GFP_NOFS, 0);
 	if (unlikely(r != 0)) {
-		jfs_err("JFS: sb_issue_discard" \
-			"(%p, %llu, %llu, GFP_NOFS, 0) = %d => failed!\n",
+		jfs_err("JFS: sb_issue_discard(%p, %llu, %llu, GFP_NOFS, 0) = %d => failed!",
 			sb, (unsigned long long)blkno,
 			(unsigned long long)nblocks, r);
 	}
 
-	jfs_info("JFS: sb_issue_discard" \
-		"(%p, %llu, %llu, GFP_NOFS, 0) = %d\n",
+	jfs_info("JFS: sb_issue_discard(%p, %llu, %llu, GFP_NOFS, 0) = %d",
 		sb, (unsigned long long)blkno,
 		(unsigned long long)nblocks, r);
 
-- 
2.8.0.rc4.16.g56331f8

[PATCH 3/3] jfs: Coalesce some formats

[Joe Perches]

Formats are better kept as a single line for easier grep.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/jfs/inode.c      |  4 ++--
 fs/jfs/jfs_dtree.c  | 10 ++++------
 fs/jfs/jfs_imap.c   |  3 +--
 fs/jfs/jfs_logmgr.c | 10 ++++------
 fs/jfs/jfs_txnmgr.c | 17 +++++++----------
 fs/jfs/namei.c      |  4 ++--
 6 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index 9d9bae6..da1d021 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -102,8 +102,8 @@ int jfs_commit_inode(struct inode *inode, int wait)
 		 * partitions and may think inode is dirty
 		 */
 		if (!special_file(inode->i_mode) && noisy) {
-			jfs_err("jfs_commit_inode(0x%p) called on "
-				   "read-only volume", inode);
+			jfs_err("jfs_commit_inode(0x%p) called on read-only volume",
+				inode);
 			jfs_err("Is remount racy?");
 			noisy--;
 		}
diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
index d88576e..de2bcb3 100644
--- a/fs/jfs/jfs_dtree.c
+++ b/fs/jfs/jfs_dtree.c
@@ -3072,8 +3072,7 @@ int jfs_readdir(struct file *file, struct dir_context *ctx)
 			}
 			if (dirtab_slot.flag == DIR_INDEX_FREE) {
 				if (loop_count++ > JFS_IP(ip)->next_index) {
-					jfs_err("jfs_readdir detected "
-						   "infinite loop!");
+					jfs_err("jfs_readdir detected infinite loop!");
 					ctx->pos = DIREND;
 					return 0;
 				}
@@ -3151,8 +3150,7 @@ int jfs_readdir(struct file *file, struct dir_context *ctx)
 				if (!dir_emit(ctx, "..", 2, PARENT(ip), DT_DIR))
 					return 0;
 			} else {
-				jfs_err("jfs_readdir called with "
-					"invalid offset!");
+				jfs_err("jfs_readdir called with invalid offset!");
 			}
 			dtoffset->pn = 1;
 			dtoffset->index = 0;
@@ -3165,8 +3163,8 @@ int jfs_readdir(struct file *file, struct dir_context *ctx)
 		}
 
 		if ((rc = dtReadNext(ip, &ctx->pos, &btstack))) {
-			jfs_err("jfs_readdir: unexpected rc = %d "
-				"from dtReadNext", rc);
+			jfs_err("jfs_readdir: unexpected rc = %d from dtReadNext",
+				rc);
 			ctx->pos = DIREND;
 			return 0;
 		}
diff --git a/fs/jfs/jfs_imap.c b/fs/jfs/jfs_imap.c
index f321986..6aca224 100644
--- a/fs/jfs/jfs_imap.c
+++ b/fs/jfs/jfs_imap.c
@@ -534,8 +534,7 @@ void diWriteSpecial(struct inode *ip, int secondary)
 	/* read the page of fixed disk inode (AIT) in raw mode */
 	mp = read_metapage(ip, address << sbi->l2nbperpage, PSIZE, 1);
 	if (mp == NULL) {
-		jfs_err("diWriteSpecial: failed to read aggregate inode "
-			"extent!");
+		jfs_err("diWriteSpecial: failed to read aggregate inode extent!");
 		return;
 	}
 
diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index 7638355..63759d7 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -1333,9 +1333,8 @@ int lmLogInit(struct jfs_log * log)
 				rc = -EINVAL;
 				goto errout20;
 			}
-			jfs_info("lmLogInit: inline log:0x%p base:0x%Lx "
-				 "size:0x%x", log,
-				 (unsigned long long) log->base, log->size);
+			jfs_info("lmLogInit: inline log:0x%p base:0x%Lx size:0x%x",
+				 log, (unsigned long long)log->base, log->size);
 		} else {
 			if (memcmp(logsuper->uuid, log->uuid, 16)) {
 				jfs_warn("wrong uuid on JFS log device");
@@ -1343,9 +1342,8 @@ int lmLogInit(struct jfs_log * log)
 			}
 			log->size = le32_to_cpu(logsuper->size);
 			log->l2bsize = le32_to_cpu(logsuper->l2bsize);
-			jfs_info("lmLogInit: external log:0x%p base:0x%Lx "
-				 "size:0x%x", log,
-				 (unsigned long long) log->base, log->size);
+			jfs_info("lmLogInit: external log:0x%p base:0x%Lx size:0x%x",
+				 log, (unsigned long long)log->base, log->size);
 		}
 
 		log->page = le32_to_cpu(logsuper->end) / LOGPSIZE;
diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
index 51421a8..eddf2b6 100644
--- a/fs/jfs/jfs_txnmgr.c
+++ b/fs/jfs/jfs_txnmgr.c
@@ -1798,8 +1798,8 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,
 			xadlock->xdlist = &p->xad[lwm];
 			tblk->xflag &= ~COMMIT_LAZY;
 		}
-		jfs_info("xtLog: alloc ip:0x%p mp:0x%p tlck:0x%p lwm:%d "
-			 "count:%d", tlck->ip, mp, tlck, lwm, xadlock->count);
+		jfs_info("xtLog: alloc ip:0x%p mp:0x%p tlck:0x%p lwm:%d count:%d",
+			 tlck->ip, mp, tlck, lwm, xadlock->count);
 
 		maplock->index = 1;
 
@@ -2025,8 +2025,7 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,
 			xadlock->count = next - lwm;
 			xadlock->xdlist = &p->xad[lwm];
 
-			jfs_info("xtLog: alloc ip:0x%p mp:0x%p count:%d "
-				 "lwm:%d next:%d",
+			jfs_info("xtLog: alloc ip:0x%p mp:0x%p count:%d lwm:%d next:%d",
 				 tlck->ip, mp, xadlock->count, lwm, next);
 			maplock->index++;
 			xadlock++;
@@ -2047,8 +2046,8 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,
 			pxdlock->count = 1;
 			pxdlock->pxd = pxd;
 
-			jfs_info("xtLog: truncate ip:0x%p mp:0x%p count:%d "
-				 "hwm:%d", ip, mp, pxdlock->count, hwm);
+			jfs_info("xtLog: truncate ip:0x%p mp:0x%p count:%d hwm:%d",
+				 ip, mp, pxdlock->count, hwm);
 			maplock->index++;
 			xadlock++;
 		}
@@ -2066,8 +2065,7 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,
 			xadlock->count = hwm - next + 1;
 			xadlock->xdlist = &p->xad[next];
 
-			jfs_info("xtLog: free ip:0x%p mp:0x%p count:%d "
-				 "next:%d hwm:%d",
+			jfs_info("xtLog: free ip:0x%p mp:0x%p count:%d next:%d hwm:%d",
 				 tlck->ip, mp, xadlock->count, next, hwm);
 			maplock->index++;
 		}
@@ -2523,8 +2521,7 @@ void txFreeMap(struct inode *ip,
 					xlen = lengthXAD(xad);
 					dbUpdatePMap(ipbmap, true, xaddr,
 						     (s64) xlen, tblk);
-					jfs_info("freePMap: xaddr:0x%lx "
-						 "xlen:%d",
+					jfs_info("freePMap: xaddr:0x%lx xlen:%d",
 						 (ulong) xaddr, xlen);
 				}
 			}
diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
index 701f893..211796a 100644
--- a/fs/jfs/namei.c
+++ b/fs/jfs/namei.c
@@ -1225,8 +1225,8 @@ static int jfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		rc = dtSearch(new_dir, &new_dname, &ino, &btstack,
 			      JFS_CREATE);
 		if (rc) {
-			jfs_err("jfs_rename didn't expect dtSearch to fail "
-				"w/rc = %d", rc);
+			jfs_err("jfs_rename didn't expect dtSearch to fail w/rc = %d",
+				rc);
 			goto out_tx;
 		}
 
-- 
2.8.0.rc4.16.g56331f8

Re: [PATCH 0/3] jfs: logging neatening

[Dave Kleikamp]

On 03/30/2016 07:23 AM, Joe Perches wrote:
> This patchset fixes the uses of jfs_info, jfs_warn and jfs_err that
> have terminating newlines and a couple other trivialities to make
> the logging a bit more consistent.

These patches look good. I'm pushing them out to the -next build.

> There is a difference in use between jfs_error and the other
> jfs_info, jfs_warn, and jfs_err logging macros.  jfs_error is more
> like the rest of the kernel and requires a newline as the last
> character of the format.
> 
> The jfs_info, jfs_warn, and jfs_err macros add the terminating
> newline to the format so the uses do not require them.

I think there's an argument for both ways of doing it. I'm sure I had my
reasons for automatically adding the newline back when I implemented
those macros. (They probably should be inline functions, but that's
another issue.)

> It is a habituated style for many people to add the terminating
> newline for many people and this difference in use between the
> various macro styles causes trivial defects in logging output.
> 
> It might be better if the jfs_info, jfs_warn, and jfs_err macros
> were changed to require a newline termination and the uses changed
> to include the newline, but that's a larger change.

Yeah, these patches are the obvious improvement, without changing
anything from a design standpoint.

> 
> Joe Perches (3):
>   jfs: Remove terminating newlines from jfs_info, jfs_warn, jfs_err uses
>   jfs: Remove unnecessary line continuations and terminating newlines
>   jfs: Coalesce some formats
> 
>  fs/jfs/inode.c       |  4 ++--
>  fs/jfs/jfs_discard.c |  6 ++----
>  fs/jfs/jfs_dtree.c   | 10 ++++------
>  fs/jfs/jfs_imap.c    |  3 +--
>  fs/jfs/jfs_inode.c   |  2 +-
>  fs/jfs/jfs_logmgr.c  | 14 ++++++--------
>  fs/jfs/jfs_txnmgr.c  | 21 +++++++++------------
>  fs/jfs/namei.c       |  4 ++--
>  fs/jfs/super.c       |  4 ++--
>  9 files changed, 29 insertions(+), 39 deletions(-)
> 

Re: [PATCH 0/3] jfs: logging neatening

[Joe Perches]

On Wed, 2016-03-30 at 10:56 -0500, Dave Kleikamp wrote:
> On 03/30/2016 07:23 AM, Joe Perches wrote:
> > 
> > This patchset fixes the uses of jfs_info, jfs_warn and jfs_err that
> > have terminating newlines and a couple other trivialities to make
> > the logging a bit more consistent.
> These patches look good. I'm pushing them out to the -next build.
> 
> > 
> > There is a difference in use between jfs_error and the other
> > jfs_info, jfs_warn, and jfs_err logging macros.  jfs_error is more
> > like the rest of the kernel and requires a newline as the last
> > character of the format.
> > 
> > The jfs_info, jfs_warn, and jfs_err macros add the terminating
> > newline to the format so the uses do not require them.
> I think there's an argument for both ways of doing it. I'm sure I had my
> reasons for automatically adding the newline back when I implemented
> those macros. (They probably should be inline functions, but that's
> another issue.)

Nah.  It was me.  I changed jfs_error awhile back to move the
newline to the uses.

commit eb8630d7d2fd13589e6a7a3ae2fe1f75f867fbed
Author: Joe Perches <joe@perches.com>
Date:   Tue Jun 4 16:39:15 2013 -0700

    jfs: Update jfs_error
    
    Use a more current logging style.
    
    Add __printf format and argument verification.
    
    Remove embedded function names from formats.
    Add %pf, __builtin_return_address(0) to jfs_error.
    Add newlines to formats for kernel style consistency.
    (One format already had an erroneous newline)
    Coalesce formats and align arguments.
    
    Object size reduced ~1KiB.
    
    $ size fs/jfs/built-in.o*
       text        data     bss     dec     hex filename
     201891       35488   63936  301315   49903 fs/jfs/built-in.o.new
     202821       35488   64192  302501   49da5 fs/jfs/built-in.o.old

Using inline functions would actually be more code as
you'd have to handle the log level and newline via
a vprintk of some type.  At least the test could be
consolidated into the inline though.

Many of the jfs_info calls appear to be function
tracing and perhaps could be eliminated altogether.

> It might be better if the jfs_info, jfs_warn, and jfs_err macros
> > were changed to require a newline termination and the uses changed
> > to include the newline, but that's a larger change.
> Yeah, these patches are the obvious improvement, without changing
> anything from a design standpoint.

Re: [PATCH 0/3] jfs: logging neatening

[Dave Kleikamp]

On 03/30/2016 11:22 AM, Joe Perches wrote:
> On Wed, 2016-03-30 at 10:56 -0500, Dave Kleikamp wrote:
>> On 03/30/2016 07:23 AM, Joe Perches wrote:
>>>
>>> There is a difference in use between jfs_error and the other
>>> jfs_info, jfs_warn, and jfs_err logging macros.  jfs_error is more
>>> like the rest of the kernel and requires a newline as the last
>>> character of the format.
>>>
>>> The jfs_info, jfs_warn, and jfs_err macros add the terminating
>>> newline to the format so the uses do not require them.
>> I think there's an argument for both ways of doing it. I'm sure I had my
>> reasons for automatically adding the newline back when I implemented
>> those macros. (They probably should be inline functions, but that's
>> another issue.)
> 
> Nah.  It was me.  I changed jfs_error awhile back to move the
> newline to the uses.
> 
> commit eb8630d7d2fd13589e6a7a3ae2fe1f75f867fbed
> Author: Joe Perches <joe@perches.com>
> Date:   Tue Jun 4 16:39:15 2013 -0700
> 
>     jfs: Update jfs_error
>     
>     Use a more current logging style.
>     
>     Add __printf format and argument verification.
>     
>     Remove embedded function names from formats.
>     Add %pf, __builtin_return_address(0) to jfs_error.
>     Add newlines to formats for kernel style consistency.
>     (One format already had an erroneous newline)
>     Coalesce formats and align arguments.
>     
>     Object size reduced ~1KiB.
>     
>     $ size fs/jfs/built-in.o*
>        text        data     bss     dec     hex filename
>      201891       35488   63936  301315   49903 fs/jfs/built-in.o.new
>      202821       35488   64192  302501   49da5 fs/jfs/built-in.o.old
> 
> Using inline functions would actually be more code as
> you'd have to handle the log level and newline via
> a vprintk of some type.  At least the test could be
> consolidated into the inline though.

Okay.

> Many of the jfs_info calls appear to be function
> tracing and perhaps could be eliminated altogether.

Yeah. They've been in there forever. Should probably have been stripped
out before the code was initially merged.