[PATCH 0/5] xfsprogs: my very own patchbomb

[PATCH 0/5] xfsprogs: my very own patchbomb

[Eric Sandeen]

Mostly resends of previously-sent patches, fixed up per
prior comments.

[PATCH 1/5] libxfs: Replace XFS_BUF_SET_PTR with xfs_buf_associate_memory

[Eric Sandeen]

We test the return value of this macro, but it returns
a side-effect which looks like failure.  Write a
userspace-libxfs-specific version of xfs_buf_associate_memory
to make this code a tad more like the kernel, with a proper
return value to boot.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libxfs/libxfs_io.h        | 12 ++++++++----
 libxlog/xfs_log_recover.c |  4 ++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index 6308a74..78b6780 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -103,10 +103,6 @@ enum xfs_buf_flags_t {	/* b_flags bits */
 #define XFS_BUF_SIZE(bp)		((bp)->b_bcount)
 #define XFS_BUF_COUNT(bp)		((bp)->b_bcount)
 #define XFS_BUF_TARGET(bp)		((bp)->b_dev)
-#define XFS_BUF_SET_PTR(bp,p,cnt)	({	\
-	(bp)->b_addr = (char *)(p);		\
-	XFS_BUF_SET_COUNT(bp,cnt);		\
-})
 
 #define XFS_BUF_SET_ADDR(bp,blk)	((bp)->b_bn = (blk))
 #define XFS_BUF_SET_COUNT(bp,cnt)	((bp)->b_bcount = (cnt))
@@ -230,4 +226,12 @@ xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
 			 cksum_offset);
 }
 
+static inline int
+xfs_buf_associate_memory(struct xfs_buf *bp, void *mem, size_t len)
+{
+	bp->b_addr = mem;
+	bp->b_bcount = len;
+	return 0;
+}
+
 #endif	/* __LIBXFS_IO_H__ */
diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
index 6bd000c..58d9182 100644
--- a/libxlog/xfs_log_recover.c
+++ b/libxlog/xfs_log_recover.c
@@ -171,14 +171,14 @@ xlog_bread_offset(
 	int		orig_len = bp->b_bcount;
 	int		error, error2;
 
-	error = XFS_BUF_SET_PTR(bp, offset, BBTOB(nbblks));
+	error = xfs_buf_associate_memory(bp, offset, BBTOB(nbblks));
 	if (error)
 		return error;
 
 	error = xlog_bread_noalign(log, blk_no, nbblks, bp);
 
 	/* must reset buffer pointer even on error */
-	error2 = XFS_BUF_SET_PTR(bp, orig_offset, orig_len);
+	error2 = xfs_buf_associate_memory(bp, orig_offset, orig_len);
 	if (error)
 		return error;
 	return error2;
-- 
1.8.3.1

[PATCH 2/5] libxfs: add function to free all buffers in bcache

[Eric Sandeen]

libxfs_bcache_purge simply moves all "free" buffers
onto the xfs_buf_freelist mru list; add a new function to
actually free them when we tear everything down, so leak
checkers don't go nuts about lots of unfreed xfs_bufs
at exit.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libxfs/init.c      |  5 ++++-
 libxfs/libxfs_io.h |  1 +
 libxfs/rdwr.c      | 18 ++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/libxfs/init.c b/libxfs/init.c
index 7bde8b7..c7d73b6 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -888,8 +888,11 @@ libxfs_umount(xfs_mount_t *mp)
 void
 libxfs_destroy(void)
 {
-	manage_zones(1);
+	/* Free everything from the buffer cache before freeing buffer zone */
+	libxfs_bcache_purge();
+	libxfs_bcache_free();
 	cache_destroy(libxfs_bcache);
+	manage_zones(1);
 }
 
 int
diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index 78b6780..6de6fcb 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -188,6 +188,7 @@ extern void	libxfs_readbuf_verify(struct xfs_buf *bp,
 			const struct xfs_buf_ops *ops);
 extern xfs_buf_t *libxfs_getsb(struct xfs_mount *, int);
 extern void	libxfs_bcache_purge(void);
+extern void	libxfs_bcache_free(void);
 extern void	libxfs_bcache_flush(void);
 extern void	libxfs_purgebuf(xfs_buf_t *);
 extern int	libxfs_bcache_overflowed(void);
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 3c5def2..81701b7 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1275,6 +1275,24 @@ libxfs_bulkrelse(
 }
 
 /*
+ * Free everything from the xfs_buf_freelist MRU, used at final teardown
+ */
+void
+libxfs_bcache_free(void)
+{
+	struct list_head	*cm_list;
+	xfs_buf_t		*bp, *next;
+
+	cm_list = &xfs_buf_freelist.cm_list;
+	list_for_each_entry_safe(bp, next, cm_list, b_node.cn_mru) {
+		free(bp->b_addr);
+		if (bp->b_maps != &bp->__b_map)
+			free(bp->b_maps);
+		kmem_zone_free(xfs_buf_zone, bp);
+	}
+}
+
+/*
  * When a buffer is marked dirty, the error is cleared. Hence if we are trying
  * to flush a buffer prior to cache reclaim that has an error on it it means
  * we've already tried to flush it and it failed. Prevent repeated corruption
-- 
1.8.3.1

[PATCH 3/5] libxfs: move xfs_inode_zone to rdwr.c

[Eric Sandeen]

The zone itself is created in rdwr.c, so define it there as
well, and add it to the list of externs in manage_zones along
with all the rest, for consistency.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libxfs/init.c | 3 +--
 libxfs/rdwr.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/libxfs/init.c b/libxfs/init.c
index c7d73b6..3456cb5 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -45,8 +45,6 @@ int	use_xfs_buf_lock;	/* global flag: use xfs_buf_t locks for MT */
 
 static void manage_zones(int);	/* setup global zones */
 
-kmem_zone_t	*xfs_inode_zone;
-
 /*
  * dev_map - map open devices to fd.
  */
@@ -379,6 +377,7 @@ manage_zones(int release)
 {
 	extern kmem_zone_t	*xfs_buf_zone;
 	extern kmem_zone_t	*xfs_ili_zone;
+	extern kmem_zone_t	*xfs_inode_zone;
 	extern kmem_zone_t	*xfs_ifork_zone;
 	extern kmem_zone_t	*xfs_buf_item_zone;
 	extern kmem_zone_t	*xfs_da_state_zone;
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 81701b7..3142094 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1351,8 +1351,8 @@ struct cache_operations libxfs_bcache_operations = {
  * Inode cache stubs.
  */
 
+kmem_zone_t		*xfs_inode_zone;
 extern kmem_zone_t	*xfs_ili_zone;
-extern kmem_zone_t	*xfs_inode_zone;
 
 /*
  * If there are inline format data / attr forks attached to this inode,
-- 
1.8.3.1

[PATCH 4/5] libxfs: Catch non-empty zones on destroy

[Eric Sandeen]

Create and use a kmem_zone_destroy which warns if we are
releasing a non-empty zone when the LIBXFS_LEAK_CHECK
environment variable is set, wire this into libxfs_destroy(),
and call that when various tools exit.

The LIBXFS_LEAK_CHECK environment variable also causes
the program to exit with failure when a leak is detected,
useful for failing automated tests if leaks are encountered.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 include/kmem.h |  1 +
 libxfs/init.c  | 36 +++++++++++++++++++++++-------------
 libxfs/kmem.c  | 14 ++++++++++++++
 3 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/include/kmem.h b/include/kmem.h
index 65f0ade..572a4fa 100644
--- a/include/kmem.h
+++ b/include/kmem.h
@@ -33,6 +33,7 @@ typedef struct kmem_zone {
 extern kmem_zone_t *kmem_zone_init(int, char *);
 extern void	*kmem_zone_alloc(kmem_zone_t *, int);
 extern void	*kmem_zone_zalloc(kmem_zone_t *, int);
+extern int	kmem_zone_destroy(kmem_zone_t *);
 
 static inline void
 kmem_zone_free(kmem_zone_t *zone, void *ptr)
diff --git a/libxfs/init.c b/libxfs/init.c
index 3456cb5..a65c86c 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -43,7 +43,7 @@ int libxfs_bhash_size;		/* #buckets in bcache */
 
 int	use_xfs_buf_lock;	/* global flag: use xfs_buf_t locks for MT */
 
-static void manage_zones(int);	/* setup global zones */
+static int manage_zones(int);	/* setup/teardown global zones */
 
 /*
  * dev_map - map open devices to fd.
@@ -372,7 +372,7 @@ done:
 /*
  * Initialize/destroy all of the zone allocators we use.
  */
-static void
+static int
 manage_zones(int release)
 {
 	extern kmem_zone_t	*xfs_buf_zone;
@@ -388,16 +388,20 @@ manage_zones(int release)
 	extern void		xfs_dir_startup();
 
 	if (release) {	/* free zone allocation */
-		kmem_free(xfs_buf_zone);
-		kmem_free(xfs_inode_zone);
-		kmem_free(xfs_ifork_zone);
-		kmem_free(xfs_buf_item_zone);
-		kmem_free(xfs_da_state_zone);
-		kmem_free(xfs_btree_cur_zone);
-		kmem_free(xfs_bmap_free_item_zone);
-		kmem_free(xfs_trans_zone);
-		kmem_free(xfs_log_item_desc_zone);
-		return;
+		int	leaked = 0;
+
+		leaked += kmem_zone_destroy(xfs_buf_zone);
+		leaked += kmem_zone_destroy(xfs_ili_zone);
+		leaked += kmem_zone_destroy(xfs_inode_zone);
+		leaked += kmem_zone_destroy(xfs_ifork_zone);
+		leaked += kmem_zone_destroy(xfs_buf_item_zone);
+		leaked += kmem_zone_destroy(xfs_da_state_zone);
+		leaked += kmem_zone_destroy(xfs_btree_cur_zone);
+		leaked += kmem_zone_destroy(xfs_bmap_free_item_zone);
+		leaked += kmem_zone_destroy(xfs_trans_zone);
+		leaked += kmem_zone_destroy(xfs_log_item_desc_zone);
+
+		return leaked;
 	}
 	/* otherwise initialise zone allocation */
 	xfs_buf_zone = kmem_zone_init(sizeof(xfs_buf_t), "xfs_buffer");
@@ -419,6 +423,8 @@ manage_zones(int release)
 	xfs_log_item_desc_zone = kmem_zone_init(
 			sizeof(struct xfs_log_item_desc), "xfs_log_item_desc");
 	xfs_dir_startup();
+
+	return 0;
 }
 
 /*
@@ -887,11 +893,15 @@ libxfs_umount(xfs_mount_t *mp)
 void
 libxfs_destroy(void)
 {
+	int	leaked;
+
 	/* Free everything from the buffer cache before freeing buffer zone */
 	libxfs_bcache_purge();
 	libxfs_bcache_free();
 	cache_destroy(libxfs_bcache);
-	manage_zones(1);
+	leaked = manage_zones(1);
+	if (getenv("LIBXFS_LEAK_CHECK") && leaked)
+		exit(1);
 }
 
 int
diff --git a/libxfs/kmem.c b/libxfs/kmem.c
index c8bcb50..a1f64e5 100644
--- a/libxfs/kmem.c
+++ b/libxfs/kmem.c
@@ -23,6 +23,20 @@ kmem_zone_init(int size, char *name)
 	return ptr;
 }
 
+int
+kmem_zone_destroy(kmem_zone_t *zone)
+{
+	int	leaked = 0;
+
+	if (getenv("LIBXFS_LEAK_CHECK") && zone->allocated) {
+		leaked = 1;
+		printf("zone %s freed with %d items allocated\n",
+				zone->zone_name, zone->allocated);
+	}
+	free(zone);
+	return leaked;
+}
+
 void *
 kmem_zone_alloc(kmem_zone_t *zone, int flags)
 {
-- 
1.8.3.1

[PATCH 5/5] Call libxfs_destroy from other utilities

[Eric Sandeen]

Call libxfs_destroy() from xfs_copy, xfs_db, mkfs.xfs, and
xfs_repair to allow us to detect leaked items in these
utilities as well.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 copy/xfs_copy.c     | 1 +
 db/init.c           | 2 ++
 mkfs/xfs_mkfs.c     | 1 +
 repair/xfs_repair.c | 1 +
 4 files changed, 5 insertions(+)

diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 16ee4d9..0b80613 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -1215,6 +1215,7 @@ main(int argc, char **argv)
 
 	check_errors();
 	libxfs_umount(mp);
+	libxfs_destroy();
 
 	return 0;
 }
diff --git a/db/init.c b/db/init.c
index b108a06..29fc344 100644
--- a/db/init.c
+++ b/db/init.c
@@ -236,5 +236,7 @@ close_devices:
 		libxfs_device_close(x.logdev);
 	if (x.rtdev)
 		libxfs_device_close(x.rtdev);
+	libxfs_destroy();
+
 	return exitcode;
 }
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f973b6b..1ca6a2d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -4045,6 +4045,7 @@ main(
 	if (xi.logdev && xi.logdev != xi.ddev)
 		libxfs_device_close(xi.logdev);
 	libxfs_device_close(xi.ddev);
+	libxfs_destroy();
 
 	return 0;
 }
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index b2dd91b..312a0d0 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -1082,6 +1082,7 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
 	if (x.logdev && x.logdev != x.ddev)
 		libxfs_device_close(x.logdev);
 	libxfs_device_close(x.ddev);
+	libxfs_destroy();
 
 	if (verbose)
 		summary_report();
-- 
1.8.3.1

Re: [PATCH 1/5] libxfs: Replace XFS_BUF_SET_PTR with xfs_buf_associate_memory

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 03:54:08PM -0600, Eric Sandeen wrote:
> We test the return value of this macro, but it returns
> a side-effect which looks like failure.  Write a
> userspace-libxfs-specific version of xfs_buf_associate_memory
> to make this code a tad more like the kernel, with a proper
> return value to boot.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  libxfs/libxfs_io.h        | 12 ++++++++----
>  libxlog/xfs_log_recover.c |  4 ++--
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> index 6308a74..78b6780 100644
> --- a/libxfs/libxfs_io.h
> +++ b/libxfs/libxfs_io.h
> @@ -103,10 +103,6 @@ enum xfs_buf_flags_t {	/* b_flags bits */
>  #define XFS_BUF_SIZE(bp)		((bp)->b_bcount)
>  #define XFS_BUF_COUNT(bp)		((bp)->b_bcount)
>  #define XFS_BUF_TARGET(bp)		((bp)->b_dev)
> -#define XFS_BUF_SET_PTR(bp,p,cnt)	({	\
> -	(bp)->b_addr = (char *)(p);		\
> -	XFS_BUF_SET_COUNT(bp,cnt);		\
> -})
>  
>  #define XFS_BUF_SET_ADDR(bp,blk)	((bp)->b_bn = (blk))
>  #define XFS_BUF_SET_COUNT(bp,cnt)	((bp)->b_bcount = (cnt))
> @@ -230,4 +226,12 @@ xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
>  			 cksum_offset);
>  }
>  
> +static inline int
> +xfs_buf_associate_memory(struct xfs_buf *bp, void *mem, size_t len)
> +{
> +	bp->b_addr = mem;
> +	bp->b_bcount = len;

So long as len is never larger than an unsigned int (len is size_t,
b_bcount is unsigned int) this is fine. :)

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> +	return 0;
> +}
> +
>  #endif	/* __LIBXFS_IO_H__ */
> diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
> index 6bd000c..58d9182 100644
> --- a/libxlog/xfs_log_recover.c
> +++ b/libxlog/xfs_log_recover.c
> @@ -171,14 +171,14 @@ xlog_bread_offset(
>  	int		orig_len = bp->b_bcount;
>  	int		error, error2;
>  
> -	error = XFS_BUF_SET_PTR(bp, offset, BBTOB(nbblks));
> +	error = xfs_buf_associate_memory(bp, offset, BBTOB(nbblks));
>  	if (error)
>  		return error;
>  
>  	error = xlog_bread_noalign(log, blk_no, nbblks, bp);
>  
>  	/* must reset buffer pointer even on error */
> -	error2 = XFS_BUF_SET_PTR(bp, orig_offset, orig_len);
> +	error2 = xfs_buf_associate_memory(bp, orig_offset, orig_len);
>  	if (error)
>  		return error;
>  	return error2;
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] libxfs: move xfs_inode_zone to rdwr.c

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 03:55:54PM -0600, Eric Sandeen wrote:
> The zone itself is created in rdwr.c, so define it there as
> well, and add it to the list of externs in manage_zones along
> with all the rest, for consistency.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  libxfs/init.c | 3 +--
>  libxfs/rdwr.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index c7d73b6..3456cb5 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -45,8 +45,6 @@ int	use_xfs_buf_lock;	/* global flag: use xfs_buf_t locks for MT */
>  
>  static void manage_zones(int);	/* setup global zones */
>  
> -kmem_zone_t	*xfs_inode_zone;
> -
>  /*
>   * dev_map - map open devices to fd.
>   */
> @@ -379,6 +377,7 @@ manage_zones(int release)
>  {
>  	extern kmem_zone_t	*xfs_buf_zone;
>  	extern kmem_zone_t	*xfs_ili_zone;
> +	extern kmem_zone_t	*xfs_inode_zone;
>  	extern kmem_zone_t	*xfs_ifork_zone;
>  	extern kmem_zone_t	*xfs_buf_item_zone;
>  	extern kmem_zone_t	*xfs_da_state_zone;
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 81701b7..3142094 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1351,8 +1351,8 @@ struct cache_operations libxfs_bcache_operations = {
>   * Inode cache stubs.
>   */
>  
> +kmem_zone_t		*xfs_inode_zone;

struct kmem_zone?

Otherwise,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


>  extern kmem_zone_t	*xfs_ili_zone;
> -extern kmem_zone_t	*xfs_inode_zone;
>  
>  /*
>   * If there are inline format data / attr forks attached to this inode,
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] libxfs: Catch non-empty zones on destroy

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 03:56:11PM -0600, Eric Sandeen wrote:
> Create and use a kmem_zone_destroy which warns if we are
> releasing a non-empty zone when the LIBXFS_LEAK_CHECK
> environment variable is set, wire this into libxfs_destroy(),
> and call that when various tools exit.
> 
> The LIBXFS_LEAK_CHECK environment variable also causes
> the program to exit with failure when a leak is detected,
> useful for failing automated tests if leaks are encountered.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  include/kmem.h |  1 +
>  libxfs/init.c  | 36 +++++++++++++++++++++++-------------
>  libxfs/kmem.c  | 14 ++++++++++++++
>  3 files changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/include/kmem.h b/include/kmem.h
> index 65f0ade..572a4fa 100644
> --- a/include/kmem.h
> +++ b/include/kmem.h
> @@ -33,6 +33,7 @@ typedef struct kmem_zone {
>  extern kmem_zone_t *kmem_zone_init(int, char *);
>  extern void	*kmem_zone_alloc(kmem_zone_t *, int);
>  extern void	*kmem_zone_zalloc(kmem_zone_t *, int);
> +extern int	kmem_zone_destroy(kmem_zone_t *);
>  
>  static inline void
>  kmem_zone_free(kmem_zone_t *zone, void *ptr)
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 3456cb5..a65c86c 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -43,7 +43,7 @@ int libxfs_bhash_size;		/* #buckets in bcache */
>  
>  int	use_xfs_buf_lock;	/* global flag: use xfs_buf_t locks for MT */
>  
> -static void manage_zones(int);	/* setup global zones */
> +static int manage_zones(int);	/* setup/teardown global zones */

Returns what?  Number of leaked objects?

Oh, 1 if leaks and 0 if no leaks.

>  
>  /*
>   * dev_map - map open devices to fd.
> @@ -372,7 +372,7 @@ done:
>  /*
>   * Initialize/destroy all of the zone allocators we use.
>   */
> -static void
> +static int
>  manage_zones(int release)
>  {
>  	extern kmem_zone_t	*xfs_buf_zone;
> @@ -388,16 +388,20 @@ manage_zones(int release)
>  	extern void		xfs_dir_startup();
>  
>  	if (release) {	/* free zone allocation */
> -		kmem_free(xfs_buf_zone);
> -		kmem_free(xfs_inode_zone);
> -		kmem_free(xfs_ifork_zone);
> -		kmem_free(xfs_buf_item_zone);
> -		kmem_free(xfs_da_state_zone);
> -		kmem_free(xfs_btree_cur_zone);
> -		kmem_free(xfs_bmap_free_item_zone);
> -		kmem_free(xfs_trans_zone);
> -		kmem_free(xfs_log_item_desc_zone);
> -		return;
> +		int	leaked = 0;
> +
> +		leaked += kmem_zone_destroy(xfs_buf_zone);
> +		leaked += kmem_zone_destroy(xfs_ili_zone);
> +		leaked += kmem_zone_destroy(xfs_inode_zone);
> +		leaked += kmem_zone_destroy(xfs_ifork_zone);
> +		leaked += kmem_zone_destroy(xfs_buf_item_zone);
> +		leaked += kmem_zone_destroy(xfs_da_state_zone);
> +		leaked += kmem_zone_destroy(xfs_btree_cur_zone);
> +		leaked += kmem_zone_destroy(xfs_bmap_free_item_zone);
> +		leaked += kmem_zone_destroy(xfs_trans_zone);
> +		leaked += kmem_zone_destroy(xfs_log_item_desc_zone);
> +
> +		return leaked;
>  	}
>  	/* otherwise initialise zone allocation */
>  	xfs_buf_zone = kmem_zone_init(sizeof(xfs_buf_t), "xfs_buffer");
> @@ -419,6 +423,8 @@ manage_zones(int release)
>  	xfs_log_item_desc_zone = kmem_zone_init(
>  			sizeof(struct xfs_log_item_desc), "xfs_log_item_desc");
>  	xfs_dir_startup();
> +
> +	return 0;
>  }
>  
>  /*
> @@ -887,11 +893,15 @@ libxfs_umount(xfs_mount_t *mp)
>  void
>  libxfs_destroy(void)
>  {
> +	int	leaked;
> +
>  	/* Free everything from the buffer cache before freeing buffer zone */
>  	libxfs_bcache_purge();
>  	libxfs_bcache_free();
>  	cache_destroy(libxfs_bcache);
> -	manage_zones(1);
> +	leaked = manage_zones(1);
> +	if (getenv("LIBXFS_LEAK_CHECK") && leaked)
> +		exit(1);

What do you think of assert(getenv() == NULL || !leaked); here?
LIBXFS_LEAK_CHECK is a debugging option, so we might as well dump core
right where we violate the assumptions.

(OTOH I guess this probably happens at program exit anyway...)

>  }
>  
>  int
> diff --git a/libxfs/kmem.c b/libxfs/kmem.c
> index c8bcb50..a1f64e5 100644
> --- a/libxfs/kmem.c
> +++ b/libxfs/kmem.c
> @@ -23,6 +23,20 @@ kmem_zone_init(int size, char *name)
>  	return ptr;
>  }
>  
> +int
> +kmem_zone_destroy(kmem_zone_t *zone)
> +{
> +	int	leaked = 0;
> +
> +	if (getenv("LIBXFS_LEAK_CHECK") && zone->allocated) {
> +		leaked = 1;
> +		printf("zone %s freed with %d items allocated\n",
> +				zone->zone_name, zone->allocated);

fprintf(stderr, ...)?  Since leaking /is/ an error.

--D

> +	}
> +	free(zone);
> +	return leaked;
> +}
> +
>  void *
>  kmem_zone_alloc(kmem_zone_t *zone, int flags)
>  {
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] Call libxfs_destroy from other utilities

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 03:56:29PM -0600, Eric Sandeen wrote:
> Call libxfs_destroy() from xfs_copy, xfs_db, mkfs.xfs, and
> xfs_repair to allow us to detect leaked items in these
> utilities as well.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  copy/xfs_copy.c     | 1 +
>  db/init.c           | 2 ++
>  mkfs/xfs_mkfs.c     | 1 +
>  repair/xfs_repair.c | 1 +
>  4 files changed, 5 insertions(+)
> 
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index 16ee4d9..0b80613 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -1215,6 +1215,7 @@ main(int argc, char **argv)
>  
>  	check_errors();
>  	libxfs_umount(mp);
> +	libxfs_destroy();
>  
>  	return 0;
>  }
> diff --git a/db/init.c b/db/init.c
> index b108a06..29fc344 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -236,5 +236,7 @@ close_devices:
>  		libxfs_device_close(x.logdev);
>  	if (x.rtdev)
>  		libxfs_device_close(x.rtdev);
> +	libxfs_destroy();
> +
>  	return exitcode;
>  }
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f973b6b..1ca6a2d 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -4045,6 +4045,7 @@ main(
>  	if (xi.logdev && xi.logdev != xi.ddev)
>  		libxfs_device_close(xi.logdev);
>  	libxfs_device_close(xi.ddev);
> +	libxfs_destroy();
>  
>  	return 0;
>  }
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index b2dd91b..312a0d0 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -1082,6 +1082,7 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
>  	if (x.logdev && x.logdev != x.ddev)
>  		libxfs_device_close(x.logdev);
>  	libxfs_device_close(x.ddev);
> +	libxfs_destroy();
>  
>  	if (verbose)
>  		summary_report();
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] libxfs: add function to free all buffers in bcache

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 03:54:51PM -0600, Eric Sandeen wrote:
> libxfs_bcache_purge simply moves all "free" buffers
> onto the xfs_buf_freelist mru list; add a new function to
> actually free them when we tear everything down, so leak
> checkers don't go nuts about lots of unfreed xfs_bufs
> at exit.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  libxfs/init.c      |  5 ++++-
>  libxfs/libxfs_io.h |  1 +
>  libxfs/rdwr.c      | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 7bde8b7..c7d73b6 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -888,8 +888,11 @@ libxfs_umount(xfs_mount_t *mp)
>  void
>  libxfs_destroy(void)
>  {
> -	manage_zones(1);
> +	/* Free everything from the buffer cache before freeing buffer zone */
> +	libxfs_bcache_purge();
> +	libxfs_bcache_free();

/me wonders if we should warn about unflushed buffers here, but afaict
all the existing programs take care of this, so...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  	cache_destroy(libxfs_bcache);
> +	manage_zones(1);
>  }
>  
>  int
> diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> index 78b6780..6de6fcb 100644
> --- a/libxfs/libxfs_io.h
> +++ b/libxfs/libxfs_io.h
> @@ -188,6 +188,7 @@ extern void	libxfs_readbuf_verify(struct xfs_buf *bp,
>  			const struct xfs_buf_ops *ops);
>  extern xfs_buf_t *libxfs_getsb(struct xfs_mount *, int);
>  extern void	libxfs_bcache_purge(void);
> +extern void	libxfs_bcache_free(void);
>  extern void	libxfs_bcache_flush(void);
>  extern void	libxfs_purgebuf(xfs_buf_t *);
>  extern int	libxfs_bcache_overflowed(void);
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 3c5def2..81701b7 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1275,6 +1275,24 @@ libxfs_bulkrelse(
>  }
>  
>  /*
> + * Free everything from the xfs_buf_freelist MRU, used at final teardown
> + */
> +void
> +libxfs_bcache_free(void)
> +{
> +	struct list_head	*cm_list;
> +	xfs_buf_t		*bp, *next;
> +
> +	cm_list = &xfs_buf_freelist.cm_list;
> +	list_for_each_entry_safe(bp, next, cm_list, b_node.cn_mru) {
> +		free(bp->b_addr);
> +		if (bp->b_maps != &bp->__b_map)
> +			free(bp->b_maps);
> +		kmem_zone_free(xfs_buf_zone, bp);
> +	}
> +}
> +
> +/*
>   * When a buffer is marked dirty, the error is cleared. Hence if we are trying
>   * to flush a buffer prior to cache reclaim that has an error on it it means
>   * we've already tried to flush it and it failed. Prevent repeated corruption
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/5] libxfs-apply: add Signed-off-by:

[Eric Sandeen]

Technically when a maintainer moves a patch from another project,
they should add their Signed-off-by: tag.  Pick up SOB info
from git-config, and add an option to override that value if
desired.

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

diff --git a/tools/libxfs-apply b/tools/libxfs-apply
index e7d7e0a..2957fa0 100755
--- a/tools/libxfs-apply
+++ b/tools/libxfs-apply
@@ -8,7 +8,7 @@ usage()
 	echo $*
 	echo
 	echo "Usage:"
-	echo "	libxfs-apply [--verbose] --source <repodir> --commit <commit_id>"
+	echo "	libxfs-apply [--verbose] [--sob <name/email>] --source <repodir> --commit <commit_id>"
 	echo "	libxfs-apply --patch <patchfile>"
 	echo
 	echo "libxfs-apply should be run in the destination git repository."
@@ -73,6 +73,7 @@ while [ $# -gt 0 ]; do
 	--source)	REPO=$2 ; shift ;;
 	--patch)	PATCH=$2; shift ;;
 	--commit)	COMMIT_ID=$2 ; shift ;;
+	--sob)		SIGNED_OFF_BY=$2 ; shift ;;
 	--verbose)	VERBOSE=true ;;
 	*)		usage ;;
 	esac
@@ -274,6 +275,20 @@ fixup_header_format()
 			print $0
 		}' > $_hdr.new
 
+	# Remove the last line if it contains only whitespace
+	sed -i '${/^ *$/d;}' $_hdr.new
+
+	# Add Signed-off-by: header if specified
+	if [ ! -z ${SIGNED_OFF_BY+x} ]; then 
+		echo "Signed-off-by: $SIGNED_OFF_BY" >> $_hdr.new
+	else	# get it from git config if present
+		SOB_NAME=`git config --get user.name`
+		SOB_EMAIL=`git config --get user.email`
+		if [ ! -z ${SOB_NAME+x} ]; then
+			echo "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" >> $_hdr.new
+		fi
+	fi
+
 	# now output the new patch
 	cat $_hdr.new $_diff
 

Re: [PATCH 4/5] libxfs: Catch non-empty zones on destroy

[Eric Sandeen]

On 3/6/18 5:06 PM, Darrick J. Wong wrote:
>> @@ -887,11 +893,15 @@ libxfs_umount(xfs_mount_t *mp)
>>  void
>>  libxfs_destroy(void)
>>  {
>> +	int	leaked;
>> +
>>  	/* Free everything from the buffer cache before freeing buffer zone */
>>  	libxfs_bcache_purge();
>>  	libxfs_bcache_free();
>>  	cache_destroy(libxfs_bcache);
>> -	manage_zones(1);
>> +	leaked = manage_zones(1);
>> +	if (getenv("LIBXFS_LEAK_CHECK") && leaked)
>> +		exit(1);
> What do you think of assert(getenv() == NULL || !leaked); here?
> LIBXFS_LEAK_CHECK is a debugging option, so we might as well dump core
> right where we violate the assumptions.
> 
> (OTOH I guess this probably happens at program exit anyway...)

I don't think a core dump will be that useful, we already print out
which zone leaked & how much - I don't think a core would tell us more...?

-Eric

Re: [PATCH 6/5] libxfs-apply: add Signed-off-by:

[Darrick J. Wong]

> Subject: [PATCH 6/5] libxfs-apply: add Signed-off-by:

Heh.  Everybody's doing it!

On Tue, Mar 06, 2018 at 05:24:16PM -0600, Eric Sandeen wrote:
> Technically when a maintainer moves a patch from another project,
> they should add their Signed-off-by: tag.  Pick up SOB info
> from git-config, and add an option to override that value if
> desired.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> --
> 
> diff --git a/tools/libxfs-apply b/tools/libxfs-apply
> index e7d7e0a..2957fa0 100755
> --- a/tools/libxfs-apply
> +++ b/tools/libxfs-apply
> @@ -8,7 +8,7 @@ usage()
>  	echo $*
>  	echo
>  	echo "Usage:"
> -	echo "	libxfs-apply [--verbose] --source <repodir> --commit <commit_id>"
> +	echo "	libxfs-apply [--verbose] [--sob <name/email>] --source <repodir> --commit <commit_id>"
>  	echo "	libxfs-apply --patch <patchfile>"
>  	echo
>  	echo "libxfs-apply should be run in the destination git repository."
> @@ -73,6 +73,7 @@ while [ $# -gt 0 ]; do
>  	--source)	REPO=$2 ; shift ;;
>  	--patch)	PATCH=$2; shift ;;
>  	--commit)	COMMIT_ID=$2 ; shift ;;
> +	--sob)		SIGNED_OFF_BY=$2 ; shift ;;

Heh.

>  	--verbose)	VERBOSE=true ;;
>  	*)		usage ;;
>  	esac
> @@ -274,6 +275,20 @@ fixup_header_format()
>  			print $0
>  		}' > $_hdr.new
>  
> +	# Remove the last line if it contains only whitespace
> +	sed -i '${/^ *$/d;}' $_hdr.new

'/^[[:space:]]*$/d' ?

Just in case my computer starts dumping tabs into the commit messages.

> +
> +	# Add Signed-off-by: header if specified
> +	if [ ! -z ${SIGNED_OFF_BY+x} ]; then 
> +		echo "Signed-off-by: $SIGNED_OFF_BY" >> $_hdr.new
> +	else	# get it from git config if present
> +		SOB_NAME=`git config --get user.name`
> +		SOB_EMAIL=`git config --get user.email`
> +		if [ ! -z ${SOB_NAME+x} ]; then
> +			echo "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" >> $_hdr.new

I don't think it's necessary to add your sob if it's already on the
kernel patch.

--D

> +		fi
> +	fi
> +
>  	# now output the new patch
>  	cat $_hdr.new $_diff
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/5] libxfs-apply: add Signed-off-by:

[Eric Sandeen]

On 3/6/18 5:32 PM, Darrick J. Wong wrote:
>> Subject: [PATCH 6/5] libxfs-apply: add Signed-off-by:
> 
> Heh.  Everybody's doing it!
> 
> On Tue, Mar 06, 2018 at 05:24:16PM -0600, Eric Sandeen wrote:
>> Technically when a maintainer moves a patch from another project,
>> they should add their Signed-off-by: tag.  Pick up SOB info
>> from git-config, and add an option to override that value if
>> desired.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> --
>>
>> diff --git a/tools/libxfs-apply b/tools/libxfs-apply
>> index e7d7e0a..2957fa0 100755
>> --- a/tools/libxfs-apply
>> +++ b/tools/libxfs-apply
>> @@ -8,7 +8,7 @@ usage()
>>  	echo $*
>>  	echo
>>  	echo "Usage:"
>> -	echo "	libxfs-apply [--verbose] --source <repodir> --commit <commit_id>"
>> +	echo "	libxfs-apply [--verbose] [--sob <name/email>] --source <repodir> --commit <commit_id>"
>>  	echo "	libxfs-apply --patch <patchfile>"
>>  	echo
>>  	echo "libxfs-apply should be run in the destination git repository."
>> @@ -73,6 +73,7 @@ while [ $# -gt 0 ]; do
>>  	--source)	REPO=$2 ; shift ;;
>>  	--patch)	PATCH=$2; shift ;;
>>  	--commit)	COMMIT_ID=$2 ; shift ;;
>> +	--sob)		SIGNED_OFF_BY=$2 ; shift ;;
> 
> Heh.
> 
>>  	--verbose)	VERBOSE=true ;;
>>  	*)		usage ;;
>>  	esac
>> @@ -274,6 +275,20 @@ fixup_header_format()
>>  			print $0
>>  		}' > $_hdr.new
>>  
>> +	# Remove the last line if it contains only whitespace
>> +	sed -i '${/^ *$/d;}' $_hdr.new
> 
> '/^[[:space:]]*$/d' ?
> 
> Just in case my computer starts dumping tabs into the commit messages.

I guess - this is filtering what it's actually doing today.... but *shrug* ok.

>> +
>> +	# Add Signed-off-by: header if specified
>> +	if [ ! -z ${SIGNED_OFF_BY+x} ]; then 
>> +		echo "Signed-off-by: $SIGNED_OFF_BY" >> $_hdr.new
>> +	else	# get it from git config if present
>> +		SOB_NAME=`git config --get user.name`
>> +		SOB_EMAIL=`git config --get user.email`
>> +		if [ ! -z ${SOB_NAME+x} ]; then
>> +			echo "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" >> $_hdr.new
> 
> I don't think it's necessary to add your sob if it's already on the
> kernel patch.

maybe not, but it doesn't hurt either, it shows the path of the patch over time.
Are you worried about an extra line?  I guess I'm not... and I sometimes use
different emails depending on the hat I'm wearing, so I'm not that excited about
figuring out fancy matching and exclusion here.

-Eric

[PATCH 4/5 V2] libxfs: Catch non-empty zones on destroy

[Eric Sandeen]

Create and use a kmem_zone_destroy which warns if we are
releasing a non-empty zone when the LIBXFS_LEAK_CHECK
environment variable is set, wire this into libxfs_destroy(),
and call that when various tools exit.

The LIBXFS_LEAK_CHECK environment variable also causes
the program to exit with failure when a leak is detected,
useful for failing automated tests if leaks are encountered.

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

v2 fprintf, stderr

 include/kmem.h |  1 +
 libxfs/init.c  | 36 +++++++++++++++++++++++-------------
 libxfs/kmem.c  | 14 ++++++++++++++
 3 files changed, 38 insertions(+), 13 deletions(-)



diff --git a/include/kmem.h b/include/kmem.h
index 65f0ade..572a4fa 100644
--- a/include/kmem.h
+++ b/include/kmem.h
@@ -33,6 +33,7 @@ typedef struct kmem_zone {
 extern kmem_zone_t *kmem_zone_init(int, char *);
 extern void	*kmem_zone_alloc(kmem_zone_t *, int);
 extern void	*kmem_zone_zalloc(kmem_zone_t *, int);
+extern int	kmem_zone_destroy(kmem_zone_t *);
 
 static inline void
 kmem_zone_free(kmem_zone_t *zone, void *ptr)
diff --git a/libxfs/init.c b/libxfs/init.c
index 3456cb5..a65c86c 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -43,7 +43,7 @@ int libxfs_bhash_size;		/* #buckets in bcache */
 
 int	use_xfs_buf_lock;	/* global flag: use xfs_buf_t locks for MT */
 
-static void manage_zones(int);	/* setup global zones */
+static int manage_zones(int);	/* setup/teardown global zones */
 
 /*
  * dev_map - map open devices to fd.
@@ -372,7 +372,7 @@ done:
 /*
  * Initialize/destroy all of the zone allocators we use.
  */
-static void
+static int
 manage_zones(int release)
 {
 	extern kmem_zone_t	*xfs_buf_zone;
@@ -388,16 +388,20 @@ manage_zones(int release)
 	extern void		xfs_dir_startup();
 
 	if (release) {	/* free zone allocation */
-		kmem_free(xfs_buf_zone);
-		kmem_free(xfs_inode_zone);
-		kmem_free(xfs_ifork_zone);
-		kmem_free(xfs_buf_item_zone);
-		kmem_free(xfs_da_state_zone);
-		kmem_free(xfs_btree_cur_zone);
-		kmem_free(xfs_bmap_free_item_zone);
-		kmem_free(xfs_trans_zone);
-		kmem_free(xfs_log_item_desc_zone);
-		return;
+		int	leaked = 0;
+
+		leaked += kmem_zone_destroy(xfs_buf_zone);
+		leaked += kmem_zone_destroy(xfs_ili_zone);
+		leaked += kmem_zone_destroy(xfs_inode_zone);
+		leaked += kmem_zone_destroy(xfs_ifork_zone);
+		leaked += kmem_zone_destroy(xfs_buf_item_zone);
+		leaked += kmem_zone_destroy(xfs_da_state_zone);
+		leaked += kmem_zone_destroy(xfs_btree_cur_zone);
+		leaked += kmem_zone_destroy(xfs_bmap_free_item_zone);
+		leaked += kmem_zone_destroy(xfs_trans_zone);
+		leaked += kmem_zone_destroy(xfs_log_item_desc_zone);
+
+		return leaked;
 	}
 	/* otherwise initialise zone allocation */
 	xfs_buf_zone = kmem_zone_init(sizeof(xfs_buf_t), "xfs_buffer");
@@ -419,6 +423,8 @@ manage_zones(int release)
 	xfs_log_item_desc_zone = kmem_zone_init(
 			sizeof(struct xfs_log_item_desc), "xfs_log_item_desc");
 	xfs_dir_startup();
+
+	return 0;
 }
 
 /*
@@ -887,11 +893,15 @@ libxfs_umount(xfs_mount_t *mp)
 void
 libxfs_destroy(void)
 {
+	int	leaked;
+
 	/* Free everything from the buffer cache before freeing buffer zone */
 	libxfs_bcache_purge();
 	libxfs_bcache_free();
 	cache_destroy(libxfs_bcache);
-	manage_zones(1);
+	leaked = manage_zones(1);
+	if (getenv("LIBXFS_LEAK_CHECK") && leaked)
+		exit(1);
 }
 
 int
diff --git a/libxfs/kmem.c b/libxfs/kmem.c
index c8bcb50..256db94 100644
--- a/libxfs/kmem.c
+++ b/libxfs/kmem.c
@@ -23,6 +23,20 @@ kmem_zone_init(int size, char *name)
 	return ptr;
 }
 
+int
+kmem_zone_destroy(kmem_zone_t *zone)
+{
+	int	leaked = 0;
+
+	if (getenv("LIBXFS_LEAK_CHECK") && zone->allocated) {
+		leaked = 1;
+		fprintf(stderr, "zone %s freed with %d items allocated\n",
+				zone->zone_name, zone->allocated);
+	}
+	free(zone);
+	return leaked;
+}
+
 void *
 kmem_zone_alloc(kmem_zone_t *zone, int flags)
 {

Re: [PATCH 4/5 V2] libxfs: Catch non-empty zones on destroy

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 05:37:06PM -0600, Eric Sandeen wrote:
> Create and use a kmem_zone_destroy which warns if we are
> releasing a non-empty zone when the LIBXFS_LEAK_CHECK
> environment variable is set, wire this into libxfs_destroy(),
> and call that when various tools exit.
> 
> The LIBXFS_LEAK_CHECK environment variable also causes
> the program to exit with failure when a leak is detected,
> useful for failing automated tests if leaks are encountered.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> v2 fprintf, stderr
> 
>  include/kmem.h |  1 +
>  libxfs/init.c  | 36 +++++++++++++++++++++++-------------
>  libxfs/kmem.c  | 14 ++++++++++++++
>  3 files changed, 38 insertions(+), 13 deletions(-)
> 
> 
> 
> diff --git a/include/kmem.h b/include/kmem.h
> index 65f0ade..572a4fa 100644
> --- a/include/kmem.h
> +++ b/include/kmem.h
> @@ -33,6 +33,7 @@ typedef struct kmem_zone {
>  extern kmem_zone_t *kmem_zone_init(int, char *);
>  extern void	*kmem_zone_alloc(kmem_zone_t *, int);
>  extern void	*kmem_zone_zalloc(kmem_zone_t *, int);
> +extern int	kmem_zone_destroy(kmem_zone_t *);
>  
>  static inline void
>  kmem_zone_free(kmem_zone_t *zone, void *ptr)
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 3456cb5..a65c86c 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -43,7 +43,7 @@ int libxfs_bhash_size;		/* #buckets in bcache */
>  
>  int	use_xfs_buf_lock;	/* global flag: use xfs_buf_t locks for MT */
>  
> -static void manage_zones(int);	/* setup global zones */
> +static int manage_zones(int);	/* setup/teardown global zones */
>  
>  /*
>   * dev_map - map open devices to fd.
> @@ -372,7 +372,7 @@ done:
>  /*
>   * Initialize/destroy all of the zone allocators we use.
>   */
> -static void
> +static int
>  manage_zones(int release)
>  {
>  	extern kmem_zone_t	*xfs_buf_zone;
> @@ -388,16 +388,20 @@ manage_zones(int release)
>  	extern void		xfs_dir_startup();
>  
>  	if (release) {	/* free zone allocation */
> -		kmem_free(xfs_buf_zone);
> -		kmem_free(xfs_inode_zone);
> -		kmem_free(xfs_ifork_zone);
> -		kmem_free(xfs_buf_item_zone);
> -		kmem_free(xfs_da_state_zone);
> -		kmem_free(xfs_btree_cur_zone);
> -		kmem_free(xfs_bmap_free_item_zone);
> -		kmem_free(xfs_trans_zone);
> -		kmem_free(xfs_log_item_desc_zone);
> -		return;
> +		int	leaked = 0;
> +
> +		leaked += kmem_zone_destroy(xfs_buf_zone);
> +		leaked += kmem_zone_destroy(xfs_ili_zone);
> +		leaked += kmem_zone_destroy(xfs_inode_zone);
> +		leaked += kmem_zone_destroy(xfs_ifork_zone);
> +		leaked += kmem_zone_destroy(xfs_buf_item_zone);
> +		leaked += kmem_zone_destroy(xfs_da_state_zone);
> +		leaked += kmem_zone_destroy(xfs_btree_cur_zone);
> +		leaked += kmem_zone_destroy(xfs_bmap_free_item_zone);
> +		leaked += kmem_zone_destroy(xfs_trans_zone);
> +		leaked += kmem_zone_destroy(xfs_log_item_desc_zone);
> +
> +		return leaked;
>  	}
>  	/* otherwise initialise zone allocation */
>  	xfs_buf_zone = kmem_zone_init(sizeof(xfs_buf_t), "xfs_buffer");
> @@ -419,6 +423,8 @@ manage_zones(int release)
>  	xfs_log_item_desc_zone = kmem_zone_init(
>  			sizeof(struct xfs_log_item_desc), "xfs_log_item_desc");
>  	xfs_dir_startup();
> +
> +	return 0;
>  }
>  
>  /*
> @@ -887,11 +893,15 @@ libxfs_umount(xfs_mount_t *mp)
>  void
>  libxfs_destroy(void)
>  {
> +	int	leaked;
> +
>  	/* Free everything from the buffer cache before freeing buffer zone */
>  	libxfs_bcache_purge();
>  	libxfs_bcache_free();
>  	cache_destroy(libxfs_bcache);
> -	manage_zones(1);
> +	leaked = manage_zones(1);
> +	if (getenv("LIBXFS_LEAK_CHECK") && leaked)
> +		exit(1);
>  }
>  
>  int
> diff --git a/libxfs/kmem.c b/libxfs/kmem.c
> index c8bcb50..256db94 100644
> --- a/libxfs/kmem.c
> +++ b/libxfs/kmem.c
> @@ -23,6 +23,20 @@ kmem_zone_init(int size, char *name)
>  	return ptr;
>  }
>  
> +int
> +kmem_zone_destroy(kmem_zone_t *zone)
> +{
> +	int	leaked = 0;
> +
> +	if (getenv("LIBXFS_LEAK_CHECK") && zone->allocated) {
> +		leaked = 1;
> +		fprintf(stderr, "zone %s freed with %d items allocated\n",
> +				zone->zone_name, zone->allocated);
> +	}
> +	free(zone);
> +	return leaked;
> +}
> +
>  void *
>  kmem_zone_alloc(kmem_zone_t *zone, int flags)
>  {
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/5] libxfs-apply: add Signed-off-by:

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 05:36:12PM -0600, Eric Sandeen wrote:
> On 3/6/18 5:32 PM, Darrick J. Wong wrote:
> >> Subject: [PATCH 6/5] libxfs-apply: add Signed-off-by:
> > 
> > Heh.  Everybody's doing it!
> > 
> > On Tue, Mar 06, 2018 at 05:24:16PM -0600, Eric Sandeen wrote:
> >> Technically when a maintainer moves a patch from another project,
> >> they should add their Signed-off-by: tag.  Pick up SOB info
> >> from git-config, and add an option to override that value if
> >> desired.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> --
> >>
> >> diff --git a/tools/libxfs-apply b/tools/libxfs-apply
> >> index e7d7e0a..2957fa0 100755
> >> --- a/tools/libxfs-apply
> >> +++ b/tools/libxfs-apply
> >> @@ -8,7 +8,7 @@ usage()
> >>  	echo $*
> >>  	echo
> >>  	echo "Usage:"
> >> -	echo "	libxfs-apply [--verbose] --source <repodir> --commit <commit_id>"
> >> +	echo "	libxfs-apply [--verbose] [--sob <name/email>] --source <repodir> --commit <commit_id>"
> >>  	echo "	libxfs-apply --patch <patchfile>"
> >>  	echo
> >>  	echo "libxfs-apply should be run in the destination git repository."
> >> @@ -73,6 +73,7 @@ while [ $# -gt 0 ]; do
> >>  	--source)	REPO=$2 ; shift ;;
> >>  	--patch)	PATCH=$2; shift ;;
> >>  	--commit)	COMMIT_ID=$2 ; shift ;;
> >> +	--sob)		SIGNED_OFF_BY=$2 ; shift ;;
> > 
> > Heh.
> > 
> >>  	--verbose)	VERBOSE=true ;;
> >>  	*)		usage ;;
> >>  	esac
> >> @@ -274,6 +275,20 @@ fixup_header_format()
> >>  			print $0
> >>  		}' > $_hdr.new
> >>  
> >> +	# Remove the last line if it contains only whitespace
> >> +	sed -i '${/^ *$/d;}' $_hdr.new
> > 
> > '/^[[:space:]]*$/d' ?
> > 
> > Just in case my computer starts dumping tabs into the commit messages.
> 
> I guess - this is filtering what it's actually doing today.... but *shrug* ok.

D'oh, I forgot that the leading '$' makes it only operate on the last
line.  Disregard this comment.

> >> +
> >> +	# Add Signed-off-by: header if specified
> >> +	if [ ! -z ${SIGNED_OFF_BY+x} ]; then 
> >> +		echo "Signed-off-by: $SIGNED_OFF_BY" >> $_hdr.new
> >> +	else	# get it from git config if present
> >> +		SOB_NAME=`git config --get user.name`
> >> +		SOB_EMAIL=`git config --get user.email`
> >> +		if [ ! -z ${SOB_NAME+x} ]; then
> >> +			echo "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" >> $_hdr.new
> > 
> > I don't think it's necessary to add your sob if it's already on the
> > kernel patch.
> 
> maybe not, but it doesn't hurt either, it shows the path of the patch over time.
> Are you worried about an extra line?  I guess I'm not... and I sometimes use
> different emails depending on the hat I'm wearing, so I'm not that excited about
> figuring out fancy matching and exclusion here.

Ok.  Good enough for me. :)

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] libxfs: Replace XFS_BUF_SET_PTR with xfs_buf_associate_memory

[Christoph Hellwig]

On Tue, Mar 06, 2018 at 03:54:08PM -0600, Eric Sandeen wrote:
> We test the return value of this macro, but it returns
> a side-effect which looks like failure.  Write a
> userspace-libxfs-specific version of xfs_buf_associate_memory
> to make this code a tad more like the kernel, with a proper
> return value to boot.

Looks good:

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

But aren't we supposed to just share xfs_log_recover.c (or the
relevant parts of it) with the kernel anyway?

Re: [PATCH 2/5] libxfs: add function to free all buffers in bcache

[Christoph Hellwig]

On Tue, Mar 06, 2018 at 03:54:51PM -0600, Eric Sandeen wrote:
> libxfs_bcache_purge simply moves all "free" buffers
> onto the xfs_buf_freelist mru list; add a new function to
> actually free them when we tear everything down, so leak
> checkers don't go nuts about lots of unfreed xfs_bufs
> at exit.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  libxfs/init.c      |  5 ++++-
>  libxfs/libxfs_io.h |  1 +
>  libxfs/rdwr.c      | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 7bde8b7..c7d73b6 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -888,8 +888,11 @@ libxfs_umount(xfs_mount_t *mp)
>  void
>  libxfs_destroy(void)
>  {
> -	manage_zones(1);
> +	/* Free everything from the buffer cache before freeing buffer zone */
> +	libxfs_bcache_purge();
> +	libxfs_bcache_free();
>  	cache_destroy(libxfs_bcache);
> +	manage_zones(1);
>  }
>  
>  int
> diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> index 78b6780..6de6fcb 100644
> --- a/libxfs/libxfs_io.h
> +++ b/libxfs/libxfs_io.h
> @@ -188,6 +188,7 @@ extern void	libxfs_readbuf_verify(struct xfs_buf *bp,
>  			const struct xfs_buf_ops *ops);
>  extern xfs_buf_t *libxfs_getsb(struct xfs_mount *, int);
>  extern void	libxfs_bcache_purge(void);
> +extern void	libxfs_bcache_free(void);
>  extern void	libxfs_bcache_flush(void);
>  extern void	libxfs_purgebuf(xfs_buf_t *);
>  extern int	libxfs_bcache_overflowed(void);
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 3c5def2..81701b7 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1275,6 +1275,24 @@ libxfs_bulkrelse(
>  }
>  
>  /*
> + * Free everything from the xfs_buf_freelist MRU, used at final teardown
> + */
> +void
> +libxfs_bcache_free(void)
> +{
> +	struct list_head	*cm_list;
> +	xfs_buf_t		*bp, *next;
> +
> +	cm_list = &xfs_buf_freelist.cm_list;

Maybe initialize cm_list on the same line as the variable declaration?

Also please don't use xfs_buf_t for new code.

Otherwise this looks fine to me:

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

Re: [PATCH 3/5] libxfs: move xfs_inode_zone to rdwr.c

[Christoph Hellwig]

> @@ -379,6 +377,7 @@ manage_zones(int release)
>  {
>  	extern kmem_zone_t	*xfs_buf_zone;
>  	extern kmem_zone_t	*xfs_ili_zone;
> +	extern kmem_zone_t	*xfs_inode_zone;

We should really aim to have externs in headers, no in .c files.

But I guess this fits the existing style and should be fixed separately,
so:

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

Re: [PATCH 4/5 V2] libxfs: Catch non-empty zones on destroy

[Christoph Hellwig]

> @@ -388,16 +388,20 @@ manage_zones(int release)
>  	extern void		xfs_dir_startup();
>  
>  	if (release) {	/* free zone allocation */

I really hate this style..

But not your fault, so:

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

Re: [PATCH 5/5] Call libxfs_destroy from other utilities

[Christoph Hellwig]

Looks good,

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