[PATCHSET 0/3] populate: fix a few bugs with fs pre-population

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCHSET 0/3] populate: fix a few bugs with fs pre-population
@ 2021-03-23  4:19 Darrick J. Wong
  2021-03-23  4:19 ` [PATCH 1/3] populate: create block devices when pre-populating filesystems Darrick J. Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Darrick J. Wong @ 2021-03-23  4:19 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

Hi all,

While I was auditing the efficacy of the xfs repair tools I noticed a
few deficiencies in the code that populates filesystems, so I fixed
them.  Most notable are the fact that we didn't create fifos, messed up
blockdev creation, and while the cache tags should record the size of
external devices, the actual device names aren't exciting.

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

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 common/populate |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

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

* [PATCH 1/3] populate: create block devices when pre-populating filesystems
  2021-03-23  4:19 [PATCHSET 0/3] populate: fix a few bugs with fs pre-population Darrick J. Wong
@ 2021-03-23  4:19 ` Darrick J. Wong
  2021-03-24 18:03   ` Christoph Hellwig
  2021-03-23  4:19 ` [PATCH 2/3] common/populate: create fifos " Darrick J. Wong
  2021-03-23  4:19 ` [PATCH 3/3] common/populate: change how we describe cached populated images Darrick J. Wong
  2 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2021-03-23  4:19 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

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

I just noticed that the fs population helper creates a chardev file
"S_IFBLK" on the scratch filesystem.  This seems bogus (particularly
since we actually also create a chardev named S_IFCHR) so fix up the
mknod calls.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/common/populate b/common/populate
index 4135d89d..8f42a528 100644
--- a/common/populate
+++ b/common/populate
@@ -230,7 +230,7 @@ _scratch_xfs_populate() {
 	# Char & block
 	echo "+ special"
 	mknod "${SCRATCH_MNT}/S_IFCHR" c 1 1
-	mknod "${SCRATCH_MNT}/S_IFBLK" c 1 1
+	mknod "${SCRATCH_MNT}/S_IFBLK" b 1 1
 
 	# special file with an xattr
 	setfacl -P -m u:nobody:r ${SCRATCH_MNT}/S_IFCHR
@@ -402,7 +402,7 @@ _scratch_ext4_populate() {
 	# Char & block
 	echo "+ special"
 	mknod "${SCRATCH_MNT}/S_IFCHR" c 1 1
-	mknod "${SCRATCH_MNT}/S_IFBLK" c 1 1
+	mknod "${SCRATCH_MNT}/S_IFBLK" b 1 1
 
 	# special file with an xattr
 	setfacl -P -m u:nobody:r ${SCRATCH_MNT}/S_IFCHR


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

* Re: [PATCH 1/3] populate: create block devices when pre-populating filesystems
  2021-03-23  4:19 ` [PATCH 1/3] populate: create block devices when pre-populating filesystems Darrick J. Wong
@ 2021-03-24 18:03   ` Christoph Hellwig
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-03-24 18:03 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: guaneryu, linux-xfs, fstests, guan

On Mon, Mar 22, 2021 at 09:19:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> I just noticed that the fs population helper creates a chardev file
> "S_IFBLK" on the scratch filesystem.  This seems bogus (particularly
> since we actually also create a chardev named S_IFCHR) so fix up the
> mknod calls.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

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

* [PATCH 2/3] common/populate: create fifos when pre-populating filesystems
  2021-03-23  4:19 [PATCHSET 0/3] populate: fix a few bugs with fs pre-population Darrick J. Wong
  2021-03-23  4:19 ` [PATCH 1/3] populate: create block devices when pre-populating filesystems Darrick J. Wong
@ 2021-03-23  4:19 ` Darrick J. Wong
  2021-03-24 18:05   ` Christoph Hellwig
  2021-03-23  4:19 ` [PATCH 3/3] common/populate: change how we describe cached populated images Darrick J. Wong
  2 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2021-03-23  4:19 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

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

Create fifos when populating the scratch filesystem for completeness.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/common/populate b/common/populate
index 8f42a528..c01b7e0e 100644
--- a/common/populate
+++ b/common/populate
@@ -231,6 +231,7 @@ _scratch_xfs_populate() {
 	echo "+ special"
 	mknod "${SCRATCH_MNT}/S_IFCHR" c 1 1
 	mknod "${SCRATCH_MNT}/S_IFBLK" b 1 1
+	mknod "${SCRATCH_MNT}/S_IFIFO" p
 
 	# special file with an xattr
 	setfacl -P -m u:nobody:r ${SCRATCH_MNT}/S_IFCHR
@@ -403,6 +404,7 @@ _scratch_ext4_populate() {
 	echo "+ special"
 	mknod "${SCRATCH_MNT}/S_IFCHR" c 1 1
 	mknod "${SCRATCH_MNT}/S_IFBLK" b 1 1
+	mknod "${SCRATCH_MNT}/S_IFIFO" p
 
 	# special file with an xattr
 	setfacl -P -m u:nobody:r ${SCRATCH_MNT}/S_IFCHR
@@ -580,6 +582,7 @@ _scratch_xfs_populate_check() {
 	extents_slink="$(__populate_find_inode "${SCRATCH_MNT}/S_IFLNK.FMT_EXTENTS")"
 	bdev="$(__populate_find_inode "${SCRATCH_MNT}/S_IFBLK")"
 	cdev="$(__populate_find_inode "${SCRATCH_MNT}/S_IFCHR")"
+	fifo="$(__populate_find_inode "${SCRATCH_MNT}/S_IFIFO")"
 	local_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_LOCAL")"
 	leaf_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_LEAF")"
 	node_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_NODE")"
@@ -605,6 +608,7 @@ _scratch_xfs_populate_check() {
 	__populate_check_xfs_dformat "${btree_dir}" "btree"
 	__populate_check_xfs_dformat "${bdev}" "dev"
 	__populate_check_xfs_dformat "${cdev}" "dev"
+	__populate_check_xfs_dformat "${fifo}" "dev"
 	__populate_check_xfs_attr "${local_attr}" "local"
 	__populate_check_xfs_attr "${leaf_attr}" "leaf"
 	__populate_check_xfs_attr "${node_attr}" "node"


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

* Re: [PATCH 2/3] common/populate: create fifos when pre-populating filesystems
  2021-03-23  4:19 ` [PATCH 2/3] common/populate: create fifos " Darrick J. Wong
@ 2021-03-24 18:05   ` Christoph Hellwig
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-03-24 18:05 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: guaneryu, linux-xfs, fstests, guan

On Mon, Mar 22, 2021 at 09:19:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Create fifos when populating the scratch filesystem for completeness.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

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

* [PATCH 3/3] common/populate: change how we describe cached populated images
  2021-03-23  4:19 [PATCHSET 0/3] populate: fix a few bugs with fs pre-population Darrick J. Wong
  2021-03-23  4:19 ` [PATCH 1/3] populate: create block devices when pre-populating filesystems Darrick J. Wong
  2021-03-23  4:19 ` [PATCH 2/3] common/populate: create fifos " Darrick J. Wong
@ 2021-03-23  4:19 ` Darrick J. Wong
  2021-03-24 18:11   ` Christoph Hellwig
  2021-03-24 18:17   ` [PATCH v1.1 " Darrick J. Wong
  2 siblings, 2 replies; 10+ messages in thread
From: Darrick J. Wong @ 2021-03-23  4:19 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

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

The device name of a secondary storage device isn't all that important,
but the size is.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)


diff --git a/common/populate b/common/populate
index c01b7e0e..94bf5ce9 100644
--- a/common/populate
+++ b/common/populate
@@ -808,13 +808,23 @@ _fill_fs()
 _scratch_populate_cache_tag() {
 	local extra_descr=""
 	local size="$(blockdev --getsz "${SCRATCH_DEV}")"
+	local logdev="none"
+	local rtdev="none"
+
+	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_LOGDEV}" ]; then
+		logdev="$(blockdev --getsz "${SCRATCH_LOGDEV}")"
+	fi
+
+	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_RTDEV}" ]; then
+		rtdev="$(blockdev --getsz "${SCRATCH_RTDEV}")"
+	fi
 
 	case "${FSTYP}" in
 	"ext4")
-		extra_descr="LOGDEV ${SCRATCH_LOGDEV} USE_EXTERNAL ${USE_EXTERNAL}"
+		extra_descr="LOGDEV_SIZE ${logdev}"
 		;;
 	"xfs")
-		extra_descr="LOGDEV ${SCRATCH_LOGDEV} USE_EXTERNAL ${USE_EXTERNAL} RTDEV ${SCRATCH_RTDEV}"
+		extra_descr="LOGDEV_SIZE ${logdev} RTDEV_SIZE ${rtdev}"
 		_populate_xfs_qmount_option
 		if echo "${MOUNT_OPTIONS}" | grep -q 'usrquota'; then
 			extra_descr="${extra_descr} QUOTAS"


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

* Re: [PATCH 3/3] common/populate: change how we describe cached populated images
  2021-03-23  4:19 ` [PATCH 3/3] common/populate: change how we describe cached populated images Darrick J. Wong
@ 2021-03-24 18:11   ` Christoph Hellwig
  2021-03-24 18:15     ` Darrick J. Wong
  2021-03-24 18:17   ` [PATCH v1.1 " Darrick J. Wong
  1 sibling, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2021-03-24 18:11 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: guaneryu, linux-xfs, fstests, guan

On Mon, Mar 22, 2021 at 09:19:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The device name of a secondary storage device isn't all that important,
> but the size is.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/populate |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index c01b7e0e..94bf5ce9 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -808,13 +808,23 @@ _fill_fs()
>  _scratch_populate_cache_tag() {
>  	local extra_descr=""
>  	local size="$(blockdev --getsz "${SCRATCH_DEV}")"
> +	local logdev="none"
> +	local rtdev="none"
> +
> +	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_LOGDEV}" ]; then
> +		logdev="$(blockdev --getsz "${SCRATCH_LOGDEV}")"
> +	fi
> +
> +	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_RTDEV}" ]; then
> +		rtdev="$(blockdev --getsz "${SCRATCH_RTDEV}")"

Shouldn't these variables be called LOGDEV_SIZE and RTDEV_SIZE?

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

* Re: [PATCH 3/3] common/populate: change how we describe cached populated images
  2021-03-24 18:11   ` Christoph Hellwig
@ 2021-03-24 18:15     ` Darrick J. Wong
  0 siblings, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2021-03-24 18:15 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: guaneryu, linux-xfs, fstests, guan

On Wed, Mar 24, 2021 at 06:11:57PM +0000, Christoph Hellwig wrote:
> On Mon, Mar 22, 2021 at 09:19:59PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The device name of a secondary storage device isn't all that important,
> > but the size is.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/populate |   14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/common/populate b/common/populate
> > index c01b7e0e..94bf5ce9 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -808,13 +808,23 @@ _fill_fs()
> >  _scratch_populate_cache_tag() {
> >  	local extra_descr=""
> >  	local size="$(blockdev --getsz "${SCRATCH_DEV}")"
> > +	local logdev="none"
> > +	local rtdev="none"
> > +
> > +	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_LOGDEV}" ]; then
> > +		logdev="$(blockdev --getsz "${SCRATCH_LOGDEV}")"
> > +	fi
> > +
> > +	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_RTDEV}" ]; then
> > +		rtdev="$(blockdev --getsz "${SCRATCH_RTDEV}")"
> 
> Shouldn't these variables be called LOGDEV_SIZE and RTDEV_SIZE?

Oops, yeah.

--D

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

* [PATCH v1.1 3/3] common/populate: change how we describe cached populated images
  2021-03-23  4:19 ` [PATCH 3/3] common/populate: change how we describe cached populated images Darrick J. Wong
  2021-03-24 18:11   ` Christoph Hellwig
@ 2021-03-24 18:17   ` Darrick J. Wong
  2021-03-24 18:22     ` Christoph Hellwig
  1 sibling, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2021-03-24 18:17 UTC (permalink / raw)
  To: guaneryu; +Cc: linux-xfs, fstests, guan, hch

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

The device name of a secondary storage device isn't all that important,
but the size is.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: fix variable names
---
 common/populate |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/common/populate b/common/populate
index c01b7e0e..d484866a 100644
--- a/common/populate
+++ b/common/populate
@@ -808,13 +808,23 @@ _fill_fs()
 _scratch_populate_cache_tag() {
 	local extra_descr=""
 	local size="$(blockdev --getsz "${SCRATCH_DEV}")"
+	local logdev_sz="none"
+	local rtdev_sz="none"
+
+	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_LOGDEV}" ]; then
+		logdev_sz="$(blockdev --getsz "${SCRATCH_LOGDEV}")"
+	fi
+
+	if [ "${USE_EXTERNAL}" = "yes" ] && [ -n "${SCRATCH_RTDEV}" ]; then
+		rtdev_sz="$(blockdev --getsz "${SCRATCH_RTDEV}")"
+	fi
 
 	case "${FSTYP}" in
 	"ext4")
-		extra_descr="LOGDEV ${SCRATCH_LOGDEV} USE_EXTERNAL ${USE_EXTERNAL}"
+		extra_descr="LOGDEV_SIZE ${logdev_sz}"
 		;;
 	"xfs")
-		extra_descr="LOGDEV ${SCRATCH_LOGDEV} USE_EXTERNAL ${USE_EXTERNAL} RTDEV ${SCRATCH_RTDEV}"
+		extra_descr="LOGDEV_SIZE ${logdev_sz} RTDEV_SIZE ${rtdev_sz}"
 		_populate_xfs_qmount_option
 		if echo "${MOUNT_OPTIONS}" | grep -q 'usrquota'; then
 			extra_descr="${extra_descr} QUOTAS"

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

* Re: [PATCH v1.1 3/3] common/populate: change how we describe cached populated images
  2021-03-24 18:17   ` [PATCH v1.1 " Darrick J. Wong
@ 2021-03-24 18:22     ` Christoph Hellwig
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-03-24 18:22 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: guaneryu, linux-xfs, fstests, guan, hch

On Wed, Mar 24, 2021 at 11:17:48AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The device name of a secondary storage device isn't all that important,
> but the size is.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

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

end of thread, other threads:[~2021-03-24 18:25 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23  4:19 [PATCHSET 0/3] populate: fix a few bugs with fs pre-population Darrick J. Wong
2021-03-23  4:19 ` [PATCH 1/3] populate: create block devices when pre-populating filesystems Darrick J. Wong
2021-03-24 18:03   ` Christoph Hellwig
2021-03-23  4:19 ` [PATCH 2/3] common/populate: create fifos " Darrick J. Wong
2021-03-24 18:05   ` Christoph Hellwig
2021-03-23  4:19 ` [PATCH 3/3] common/populate: change how we describe cached populated images Darrick J. Wong
2021-03-24 18:11   ` Christoph Hellwig
2021-03-24 18:15     ` Darrick J. Wong
2021-03-24 18:17   ` [PATCH v1.1 " Darrick J. Wong
2021-03-24 18:22     ` Christoph Hellwig

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.