* [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
* 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 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 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 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
* [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 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 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 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
* 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
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.