All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHSET 0/2] xfs: various small fixes
@ 2021-06-16 23:55 Darrick J. Wong
  2021-06-16 23:55 ` [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions Darrick J. Wong
  2021-06-16 23:55 ` [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer Darrick J. Wong
  0 siblings, 2 replies; 12+ messages in thread
From: Darrick J. Wong @ 2021-06-16 23:55 UTC (permalink / raw)
  To: djwong; +Cc: linux-xfs

Hi all,

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

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  |    2 +-
 fs/xfs/xfs_icache.c |    8 ++++----
 fs/xfs/xfs_icache.h |    6 +++---
 fs/xfs/xfs_trace.h  |    4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions
  2021-06-16 23:55 [PATCHSET 0/2] xfs: various small fixes Darrick J. Wong
@ 2021-06-16 23:55 ` Darrick J. Wong
  2021-06-17  7:58   ` Christoph Hellwig
                     ` (2 more replies)
  2021-06-16 23:55 ` [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer Darrick J. Wong
  1 sibling, 3 replies; 12+ messages in thread
From: Darrick J. Wong @ 2021-06-16 23:55 UTC (permalink / raw)
  To: djwong; +Cc: linux-xfs

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>
---
 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..18dae6d3d69a 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	= max_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,


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer
  2021-06-16 23:55 [PATCHSET 0/2] xfs: various small fixes Darrick J. Wong
  2021-06-16 23:55 ` [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions Darrick J. Wong
@ 2021-06-16 23:55 ` Darrick J. Wong
  2021-06-17  8:11   ` Christoph Hellwig
  2021-06-18 14:34   ` Brian Foster
  1 sibling, 2 replies; 12+ messages in thread
From: Darrick J. Wong @ 2021-06-16 23:55 UTC (permalink / raw)
  To: djwong; +Cc: linux-xfs

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>
---
 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) {


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions
  2021-06-16 23:55 ` [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions Darrick J. Wong
@ 2021-06-17  7:58   ` Christoph Hellwig
  2021-06-17 15:14   ` Chandan Babu R
  2021-06-18 14:33   ` Brian Foster
  2 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2021-06-17  7:58 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 04:55:30PM -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>

Looks good,

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer
  2021-06-16 23:55 ` [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer Darrick J. Wong
@ 2021-06-17  8:11   ` Christoph Hellwig
  2021-06-17 16:10     ` Darrick J. Wong
  2021-06-18 14:34   ` Brian Foster
  1 sibling, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-06-17  8:11 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 04:55:36PM -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

Symbolic names looks very useful here.  But can we take a step back
to make this whole printk much more useful, something like:

XFS (sda): Forced shutdown (log I/O error) at fs/xfs/xfs_trans.c:385 (xfs_trans_mod_sb+0x25).

That is print the reason as a string, and mke the whole thing less
verbose and more readable.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions
  2021-06-16 23:55 ` [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions Darrick J. Wong
  2021-06-17  7:58   ` Christoph Hellwig
@ 2021-06-17 15:14   ` Chandan Babu R
  2021-06-18 14:33   ` Brian Foster
  2 siblings, 0 replies; 12+ messages in thread
From: Chandan Babu R @ 2021-06-17 15:14 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On 17 Jun 2021 at 05:25, 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.
>

The changes look good to me.

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

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  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..18dae6d3d69a 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	= max_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,


-- 
chandan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer
  2021-06-17  8:11   ` Christoph Hellwig
@ 2021-06-17 16:10     ` Darrick J. Wong
  2021-06-18 13:58       ` Christoph Hellwig
  0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2021-06-17 16:10 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Thu, Jun 17, 2021 at 09:11:07AM +0100, Christoph Hellwig wrote:
> On Wed, Jun 16, 2021 at 04:55:36PM -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
> 
> Symbolic names looks very useful here.  But can we take a step back
> to make this whole printk much more useful, something like:
> 
> XFS (sda): Forced shutdown (log I/O error) at fs/xfs/xfs_trans.c:385 (xfs_trans_mod_sb+0x25).
> 
> That is print the reason as a string, and mke the whole thing less
> verbose and more readable.

Um, we /do/ log the error already; a full shutdown report looks like:

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
XFS (sda): Corruption of in-memory data detected.  Shutting down
	filesystem

Or are you saying that we should combine them into a single message?

XFS (sda): Corruption of in-memory data detected at xlog_write+0x10
	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.

XFS (sda): Log I/O error detected at xlog_write+0x10
	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.

XFS (sda): I/O error detected at xlog_write+0x10
	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.

etc?

--D

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer
  2021-06-17 16:10     ` Darrick J. Wong
@ 2021-06-18 13:58       ` Christoph Hellwig
  2021-06-18 16:05         ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-06-18 13:58 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs

On Thu, Jun 17, 2021 at 09:10:41AM -0700, Darrick J. Wong wrote:
> Um, we /do/ log the error already; a full shutdown report looks like:
> 
> 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
> XFS (sda): Corruption of in-memory data detected.  Shutting down
> 	filesystem

Yeah, it's pretty verbose.

> Or are you saying that we should combine them into a single message?
> 
> XFS (sda): Corruption of in-memory data detected at xlog_write+0x10
> 	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.
> 
> XFS (sda): Log I/O error detected at xlog_write+0x10
> 	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.
> 
> XFS (sda): I/O error detected at xlog_write+0x10
> 	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.
> 
> etc?

I think that reads much nicer.  So if we cause some churn in this area
we might as well go with that.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions
  2021-06-16 23:55 ` [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions Darrick J. Wong
  2021-06-17  7:58   ` Christoph Hellwig
  2021-06-17 15:14   ` Chandan Babu R
@ 2021-06-18 14:33   ` Brian Foster
  2021-06-18 14:59     ` Darrick J. Wong
  2 siblings, 1 reply; 12+ messages in thread
From: Brian Foster @ 2021-06-18 14:33 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 04:55:30PM -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>
> ---
>  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..18dae6d3d69a 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	= max_t(unsigned long, LONG_MAX, nr_to_scan),

Does this intend to assign LONG_MAX if nr_to_scan might be smaller?

Brian

>  	};
>  
>  	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,
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer
  2021-06-16 23:55 ` [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer Darrick J. Wong
  2021-06-17  8:11   ` Christoph Hellwig
@ 2021-06-18 14:34   ` Brian Foster
  1 sibling, 0 replies; 12+ messages in thread
From: Brian Foster @ 2021-06-18 14:34 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 04:55:36PM -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>
> ---

No objection to what Christoph suggests, but ISTM it could be a tack on
patch to improving the existing message:

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) {
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions
  2021-06-18 14:33   ` Brian Foster
@ 2021-06-18 14:59     ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2021-06-18 14:59 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Fri, Jun 18, 2021 at 10:33:58AM -0400, Brian Foster wrote:
> On Wed, Jun 16, 2021 at 04:55:30PM -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>
> > ---
> >  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..18dae6d3d69a 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	= max_t(unsigned long, LONG_MAX, nr_to_scan),
> 
> Does this intend to assign LONG_MAX if nr_to_scan might be smaller?

DOH.  Yes, this should be min_t().  Why do I always screw that up?

--D

> Brian
> 
> >  	};
> >  
> >  	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,
> > 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer
  2021-06-18 13:58       ` Christoph Hellwig
@ 2021-06-18 16:05         ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2021-06-18 16:05 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Jun 18, 2021 at 02:58:46PM +0100, Christoph Hellwig wrote:
> On Thu, Jun 17, 2021 at 09:10:41AM -0700, Darrick J. Wong wrote:
> > Um, we /do/ log the error already; a full shutdown report looks like:
> > 
> > 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
> > XFS (sda): Corruption of in-memory data detected.  Shutting down
> > 	filesystem
> 
> Yeah, it's pretty verbose.
> 
> > Or are you saying that we should combine them into a single message?
> > 
> > XFS (sda): Corruption of in-memory data detected at xlog_write+0x10
> > 	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.
> > 
> > XFS (sda): Log I/O error detected at xlog_write+0x10
> > 	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.
> > 
> > XFS (sda): I/O error detected at xlog_write+0x10
> > 	(fs/xfs/xfs_log.c:2440).  Shutting down filesystem.
> > 
> > etc?
> 
> I think that reads much nicer.  So if we cause some churn in this area
> we might as well go with that.

Ok, I'll prepare a new patch to consolidate all that.

--D

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-06-18 16:05 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 23:55 [PATCHSET 0/2] xfs: various small fixes Darrick J. Wong
2021-06-16 23:55 ` [PATCH 1/2] xfs: fix type mismatches in the inode reclaim functions Darrick J. Wong
2021-06-17  7:58   ` Christoph Hellwig
2021-06-17 15:14   ` Chandan Babu R
2021-06-18 14:33   ` Brian Foster
2021-06-18 14:59     ` Darrick J. Wong
2021-06-16 23:55 ` [PATCH 2/2] xfs: print name of function causing fs shutdown instead of hex pointer Darrick J. Wong
2021-06-17  8:11   ` Christoph Hellwig
2021-06-17 16:10     ` Darrick J. Wong
2021-06-18 13:58       ` Christoph Hellwig
2021-06-18 16:05         ` Darrick J. Wong
2021-06-18 14:34   ` Brian Foster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.