[PATCHSET v2 0/3] xfs: various small fixes and cleanups

[PATCHSET v2 0/3] xfs: various small fixes and cleanups

[Darrick J. Wong]

Hi all,

I found a couple of small things that looked like they needed cleaning
while debugging other problems in my development tree.

v2: more cleanups for shutdown messaging

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes-5.14
---
 fs/xfs/xfs_fsops.c  |   16 ++++++++--------
 fs/xfs/xfs_icache.c |    8 ++++----
 fs/xfs/xfs_icache.h |    6 +++---
 fs/xfs/xfs_trace.h  |    4 ++--
 4 files changed, 17 insertions(+), 17 deletions(-)

[PATCH 1/3] xfs: fix type mismatches in the inode reclaim functions

[Darrick J. Wong]

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

It's currently unlikely that we will ever end up with more than 4
billion inodes waiting for reclamation, but the fs object code uses long
int for object counts and we're certainly capable of generating that
many.  Instead of truncating the internal counters, widen them and
report the object counts correctly.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/xfs_icache.c |    8 ++++----
 fs/xfs/xfs_icache.h |    6 +++---
 fs/xfs/xfs_trace.h  |    4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 6b44fc734cb5..6007683482c6 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1084,11 +1084,11 @@ xfs_reclaim_inodes(
 long
 xfs_reclaim_inodes_nr(
 	struct xfs_mount	*mp,
-	int			nr_to_scan)
+	unsigned long		nr_to_scan)
 {
 	struct xfs_icwalk	icw = {
 		.icw_flags	= XFS_ICWALK_FLAG_SCAN_LIMIT,
-		.icw_scan_limit	= nr_to_scan,
+		.icw_scan_limit	= min_t(unsigned long, LONG_MAX, nr_to_scan),
 	};
 
 	if (xfs_want_reclaim_sick(mp))
@@ -1106,13 +1106,13 @@ xfs_reclaim_inodes_nr(
  * Return the number of reclaimable inodes in the filesystem for
  * the shrinker to determine how much to reclaim.
  */
-int
+long
 xfs_reclaim_inodes_count(
 	struct xfs_mount	*mp)
 {
 	struct xfs_perag	*pag;
 	xfs_agnumber_t		ag = 0;
-	int			reclaimable = 0;
+	long			reclaimable = 0;
 
 	while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_RECLAIM_TAG))) {
 		ag = pag->pag_agno + 1;
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 00dc98a92835..c751cc32dc46 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -15,7 +15,7 @@ struct xfs_icwalk {
 	kgid_t		icw_gid;
 	prid_t		icw_prid;
 	__u64		icw_min_file_size;
-	int		icw_scan_limit;
+	long		icw_scan_limit;
 };
 
 /* Flags that reflect xfs_fs_eofblocks functionality. */
@@ -49,8 +49,8 @@ void xfs_inode_free(struct xfs_inode *ip);
 void xfs_reclaim_worker(struct work_struct *work);
 
 void xfs_reclaim_inodes(struct xfs_mount *mp);
-int xfs_reclaim_inodes_count(struct xfs_mount *mp);
-long xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
+long xfs_reclaim_inodes_count(struct xfs_mount *mp);
+long xfs_reclaim_inodes_nr(struct xfs_mount *mp, unsigned long nr_to_scan);
 
 void xfs_inode_mark_reclaimable(struct xfs_inode *ip);
 
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 428dc71f7f8b..85fa864f8e2f 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3894,7 +3894,7 @@ DECLARE_EVENT_CLASS(xfs_icwalk_class,
 		__field(uint32_t, gid)
 		__field(prid_t, prid)
 		__field(__u64, min_file_size)
-		__field(int, scan_limit)
+		__field(long, scan_limit)
 		__field(unsigned long, caller_ip)
 	),
 	TP_fast_assign(
@@ -3909,7 +3909,7 @@ DECLARE_EVENT_CLASS(xfs_icwalk_class,
 		__entry->scan_limit = icw ? icw->icw_scan_limit : 0;
 		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d flags 0x%x uid %u gid %u prid %u minsize %llu scan_limit %d caller %pS",
+	TP_printk("dev %d:%d flags 0x%x uid %u gid %u prid %u minsize %llu scan_limit %ld caller %pS",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->flags,
 		  __entry->uid,

Re: [PATCH 1/3] xfs: fix type mismatches in the inode reclaim functions

[Dave Chinner]

On Fri, Jun 18, 2021 at 11:53:59AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> It's currently unlikely that we will ever end up with more than 4
> billion inodes waiting for reclamation, but the fs object code uses long
> int for object counts and we're certainly capable of generating that
> many.  Instead of truncating the internal counters, widen them and
> report the object counts correctly.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

[PATCH 2/3] xfs: print name of function causing fs shutdown instead of hex pointer

[Darrick J. Wong]

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

In xfs_do_force_shutdown, print the symbolic name of the function that
called us to shut down the filesystem instead of a raw hex pointer.
This makes debugging a lot easier:

XFS (sda): xfs_do_force_shutdown(0x2) called from line 2440 of file
	fs/xfs/xfs_log.c. Return address = ffffffffa038bc38

becomes:

XFS (sda): xfs_do_force_shutdown(0x2) called from line 2440 of file
	fs/xfs/xfs_log.c. Return address = xfs_trans_mod_sb+0x25

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_fsops.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 07c745cd483e..b7f979eca1e2 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -543,7 +543,7 @@ xfs_do_force_shutdown(
 	}
 
 	xfs_notice(mp,
-"%s(0x%x) called from line %d of file %s. Return address = "PTR_FMT,
+"%s(0x%x) called from line %d of file %s. Return address = %pS",
 		__func__, flags, lnnum, fname, __return_address);
 
 	if (flags & SHUTDOWN_CORRUPT_INCORE) {

Re: [PATCH 2/3] xfs: print name of function causing fs shutdown instead of hex pointer

[Dave Chinner]

On Fri, Jun 18, 2021 at 11:54:05AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In xfs_do_force_shutdown, print the symbolic name of the function that
> called us to shut down the filesystem instead of a raw hex pointer.
> This makes debugging a lot easier:
> 
> XFS (sda): xfs_do_force_shutdown(0x2) called from line 2440 of file
> 	fs/xfs/xfs_log.c. Return address = ffffffffa038bc38
> 
> becomes:
> 
> XFS (sda): xfs_do_force_shutdown(0x2) called from line 2440 of file
> 	fs/xfs/xfs_log.c. Return address = xfs_trans_mod_sb+0x25
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Makes sense.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/3] xfs: print name of function causing fs shutdown instead of hex pointer

[Chandan Babu R]

On 19 Jun 2021 at 00:24, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> In xfs_do_force_shutdown, print the symbolic name of the function that
> called us to shut down the filesystem instead of a raw hex pointer.
> This makes debugging a lot easier:
>
> XFS (sda): xfs_do_force_shutdown(0x2) called from line 2440 of file
> 	fs/xfs/xfs_log.c. Return address = ffffffffa038bc38
>
> becomes:
>
> XFS (sda): xfs_do_force_shutdown(0x2) called from line 2440 of file
> 	fs/xfs/xfs_log.c. Return address = xfs_trans_mod_sb+0x25
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_fsops.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 07c745cd483e..b7f979eca1e2 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -543,7 +543,7 @@ xfs_do_force_shutdown(
>  	}
>  
>  	xfs_notice(mp,
> -"%s(0x%x) called from line %d of file %s. Return address = "PTR_FMT,
> +"%s(0x%x) called from line %d of file %s. Return address = %pS",
>  		__func__, flags, lnnum, fname, __return_address);
>  
>  	if (flags & SHUTDOWN_CORRUPT_INCORE) {


-- 
chandan

Re: [PATCH 2/3] xfs: print name of function causing fs shutdown instead of hex pointer

[Christoph Hellwig]

Looks good,

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

[PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Darrick J. Wong]

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

Consolidate the shutdown messages to a single line containing the
reason, the passed-in flags, the source of the shutdown, and the end
result.  This means we now only have one line to look for when
debugging, which is useful when the fs goes down while something else is
flooding dmesg.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_fsops.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index b7f979eca1e2..6ed29b158312 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -538,25 +538,25 @@ xfs_do_force_shutdown(
 
 	if (flags & SHUTDOWN_FORCE_UMOUNT) {
 		xfs_alert(mp,
-"User initiated shutdown received. Shutting down filesystem");
+"User initiated shutdown (0x%x) received. Shutting down filesystem",
+				flags);
 		return;
 	}
 
-	xfs_notice(mp,
-"%s(0x%x) called from line %d of file %s. Return address = %pS",
-		__func__, flags, lnnum, fname, __return_address);
-
 	if (flags & SHUTDOWN_CORRUPT_INCORE) {
 		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_CORRUPT,
-"Corruption of in-memory data detected.  Shutting down filesystem");
+"Corruption of in-memory data (0x%x) detected at %pS (%s:%d).  Shutting down filesystem",
+				flags, __return_address, fname, lnnum);
 		if (XFS_ERRLEVEL_HIGH <= xfs_error_level)
 			xfs_stack_trace();
 	} else if (logerror) {
 		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_LOGERROR,
-			"Log I/O Error Detected. Shutting down filesystem");
+"Log I/O error (0x%x) detected at %pS (%s:%d). Shutting down filesystem",
+				flags, __return_address, fname, lnnum);
 	} else {
 		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_IOERROR,
-			"I/O Error Detected. Shutting down filesystem");
+"I/O error (0x%x) detected at %pS (%s:%d). Shutting down filesystem",
+				flags, __return_address, fname, lnnum);
 	}
 
 	xfs_alert(mp,

Re: [PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Dave Chinner]

On Fri, Jun 18, 2021 at 11:54:10AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Consolidate the shutdown messages to a single line containing the
> reason, the passed-in flags, the source of the shutdown, and the end
> result.  This means we now only have one line to look for when
> debugging, which is useful when the fs goes down while something else is
> flooding dmesg.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Seems reasonable. There's enough commonality for log scrapers to
easily be able to grab either old or new messages.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Chandan Babu R]

On 19 Jun 2021 at 00:24, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Consolidate the shutdown messages to a single line containing the
> reason, the passed-in flags, the source of the shutdown, and the end
> result.  This means we now only have one line to look for when
> debugging, which is useful when the fs goes down while something else is
> flooding dmesg.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_fsops.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index b7f979eca1e2..6ed29b158312 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -538,25 +538,25 @@ xfs_do_force_shutdown(
>  
>  	if (flags & SHUTDOWN_FORCE_UMOUNT) {
>  		xfs_alert(mp,
> -"User initiated shutdown received. Shutting down filesystem");
> +"User initiated shutdown (0x%x) received. Shutting down filesystem",
> +				flags);
>  		return;
>  	}
>  
> -	xfs_notice(mp,
> -"%s(0x%x) called from line %d of file %s. Return address = %pS",
> -		__func__, flags, lnnum, fname, __return_address);
> -
>  	if (flags & SHUTDOWN_CORRUPT_INCORE) {
>  		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_CORRUPT,
> -"Corruption of in-memory data detected.  Shutting down filesystem");
> +"Corruption of in-memory data (0x%x) detected at %pS (%s:%d).  Shutting down filesystem",
> +				flags, __return_address, fname, lnnum);
>  		if (XFS_ERRLEVEL_HIGH <= xfs_error_level)
>  			xfs_stack_trace();
>  	} else if (logerror) {
>  		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_LOGERROR,
> -			"Log I/O Error Detected. Shutting down filesystem");
> +"Log I/O error (0x%x) detected at %pS (%s:%d). Shutting down filesystem",
> +				flags, __return_address, fname, lnnum);
>  	} else {
>  		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_IOERROR,
> -			"I/O Error Detected. Shutting down filesystem");
> +"I/O error (0x%x) detected at %pS (%s:%d). Shutting down filesystem",
> +				flags, __return_address, fname, lnnum);
>  	}
>  
>  	xfs_alert(mp,


-- 
chandan

Re: [PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Christoph Hellwig]

On Fri, Jun 18, 2021 at 11:54:10AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Consolidate the shutdown messages to a single line containing the
> reason, the passed-in flags, the source of the shutdown, and the end
> result.  This means we now only have one line to look for when
> debugging, which is useful when the fs goes down while something else is
> flooding dmesg.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_fsops.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index b7f979eca1e2..6ed29b158312 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -538,25 +538,25 @@ xfs_do_force_shutdown(
>  
>  	if (flags & SHUTDOWN_FORCE_UMOUNT) {
>  		xfs_alert(mp,
> -"User initiated shutdown received. Shutting down filesystem");
> +"User initiated shutdown (0x%x) received. Shutting down filesystem",
> +				flags);
>  		return;
>  	}

So SHUTDOWN_FORCE_UMOUNT can actually be used together with
SHUTDOWN_LOG_IO_ERROR so printing something more specific could be
useful, although I'd prefer text over the hex flags.

>  	if (flags & SHUTDOWN_CORRUPT_INCORE) {
>  		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_CORRUPT,
> -"Corruption of in-memory data detected.  Shutting down filesystem");
> +"Corruption of in-memory data (0x%x) detected at %pS (%s:%d).  Shutting down filesystem",
> +				flags, __return_address, fname, lnnum);
>  		if (XFS_ERRLEVEL_HIGH <= xfs_error_level)
>  			xfs_stack_trace();
>  	} else if (logerror) {
>  		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_LOGERROR,
> -			"Log I/O Error Detected. Shutting down filesystem");
> +"Log I/O error (0x%x) detected at %pS (%s:%d). Shutting down filesystem",
> +				flags, __return_address, fname, lnnum);
>  	} else {
>  		xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_IOERROR,
> -			"I/O Error Detected. Shutting down filesystem");
> +"I/O error (0x%x) detected at %pS (%s:%d). Shutting down filesystem",
> +				flags, __return_address, fname, lnnum);
>  	}

However once we get here, flags can have exactly one specific value,
so printing it (especially as unreadable hex value) is completely
pointless.

Re: [PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Dave Chinner]

On Mon, Jun 21, 2021 at 06:30:27AM +0100, Christoph Hellwig wrote:
> On Fri, Jun 18, 2021 at 11:54:10AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Consolidate the shutdown messages to a single line containing the
> > reason, the passed-in flags, the source of the shutdown, and the end
> > result.  This means we now only have one line to look for when
> > debugging, which is useful when the fs goes down while something else is
> > flooding dmesg.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_fsops.c |   16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index b7f979eca1e2..6ed29b158312 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -538,25 +538,25 @@ xfs_do_force_shutdown(
> >  
> >  	if (flags & SHUTDOWN_FORCE_UMOUNT) {
> >  		xfs_alert(mp,
> > -"User initiated shutdown received. Shutting down filesystem");
> > +"User initiated shutdown (0x%x) received. Shutting down filesystem",
> > +				flags);
> >  		return;
> >  	}
> 
> So SHUTDOWN_FORCE_UMOUNT can actually be used together with
> SHUTDOWN_LOG_IO_ERROR so printing something more specific could be
> useful, although I'd prefer text over the hex flags.

I'm in the process of reworking the shutdown code because shutdown
is so, so very broken. Can we just fix the message and stop moving
the goal posts on me while I try to fix bugs?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Christoph Hellwig]

On Mon, Jun 21, 2021 at 04:02:22PM +1000, Dave Chinner wrote:
> > >  	if (flags & SHUTDOWN_FORCE_UMOUNT) {
> > >  		xfs_alert(mp,
> > > -"User initiated shutdown received. Shutting down filesystem");
> > > +"User initiated shutdown (0x%x) received. Shutting down filesystem",
> > > +				flags);
> > >  		return;
> > >  	}
> > 
> > So SHUTDOWN_FORCE_UMOUNT can actually be used together with
> > SHUTDOWN_LOG_IO_ERROR so printing something more specific could be
> > useful, although I'd prefer text over the hex flags.
> 
> I'm in the process of reworking the shutdown code because shutdown
> is so, so very broken. Can we just fix the message and stop moving
> the goal posts on me while I try to fix bugs?

I suggest just not adding these not very useful flags.  That is not
moving the goal post.  And I'm growing really tried of this pointlessly
aggressive attitude.

Re: [PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Dave Chinner]

On Mon, Jun 21, 2021 at 07:06:40AM +0100, Christoph Hellwig wrote:
> On Mon, Jun 21, 2021 at 04:02:22PM +1000, Dave Chinner wrote:
> > > >  	if (flags & SHUTDOWN_FORCE_UMOUNT) {
> > > >  		xfs_alert(mp,
> > > > -"User initiated shutdown received. Shutting down filesystem");
> > > > +"User initiated shutdown (0x%x) received. Shutting down filesystem",
> > > > +				flags);
> > > >  		return;
> > > >  	}
> > > 
> > > So SHUTDOWN_FORCE_UMOUNT can actually be used together with
> > > SHUTDOWN_LOG_IO_ERROR so printing something more specific could be
> > > useful, although I'd prefer text over the hex flags.
> > 
> > I'm in the process of reworking the shutdown code because shutdown
> > is so, so very broken. Can we just fix the message and stop moving
> > the goal posts on me while I try to fix bugs?
> 
> I suggest just not adding these not very useful flags.  That is not
> moving the goal post.  And I'm growing really tried of this pointlessly
> aggressive attitude.

Aggressive? Not at all. I'm being realistic.

We've still got bugs in the for-next tree that need to be fixed and
this code is part of the problem. It's already -rc7 and we need to
focus on understanding the bugs in for-next well enough to either
fix them or revert them.

Cosmetic concerns about the code are extremely low priority right
now, so can you please just have a little patience here and wait for
me to deal with the bugs rather than bikeshedding log messages that
might not even exist in a couple of days time?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: shorten the shutdown messages to a single line

[Darrick J. Wong]

On Mon, Jun 21, 2021 at 04:29:29PM +1000, Dave Chinner wrote:
> On Mon, Jun 21, 2021 at 07:06:40AM +0100, Christoph Hellwig wrote:
> > On Mon, Jun 21, 2021 at 04:02:22PM +1000, Dave Chinner wrote:
> > > > >  	if (flags & SHUTDOWN_FORCE_UMOUNT) {
> > > > >  		xfs_alert(mp,
> > > > > -"User initiated shutdown received. Shutting down filesystem");
> > > > > +"User initiated shutdown (0x%x) received. Shutting down filesystem",
> > > > > +				flags);
> > > > >  		return;
> > > > >  	}
> > > > 
> > > > So SHUTDOWN_FORCE_UMOUNT can actually be used together with
> > > > SHUTDOWN_LOG_IO_ERROR so printing something more specific could be
> > > > useful, although I'd prefer text over the hex flags.
> > > 
> > > I'm in the process of reworking the shutdown code because shutdown
> > > is so, so very broken. Can we just fix the message and stop moving
> > > the goal posts on me while I try to fix bugs?
> > 
> > I suggest just not adding these not very useful flags.  That is not
> > moving the goal post.  And I'm growing really tried of this pointlessly
> > aggressive attitude.
> 
> Aggressive? Not at all. I'm being realistic.
> 
> We've still got bugs in the for-next tree that need to be fixed and
> this code is part of the problem. It's already -rc7 and we need to
> focus on understanding the bugs in for-next well enough to either
> fix them or revert them.
> 
> Cosmetic concerns about the code are extremely low priority right
> now, so can you please just have a little patience here and wait for
> me to deal with the bugs rather than bikeshedding log messages that
> might not even exist in a couple of days time?

FWIW I /did/ notice that the flags usage could be turned into an enum
and intentionally left that cleanup (and the "int logerror" sprinkled
everywhere) for 5.15.  Maybe Dave will beat me to it, who knows.

--D

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com