[PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets"

[PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets"

[Gao Xiang]

Bind mount mntpnts will be forbided in the next commits
instead since it's not the real rootdir.

This cannot be reverted cleanly due to several cleanup
patches, but the logic is reverted equivalently.

This reverts commit 25195ebf107dc81b1b7cea1476764950e1d6cc9d.

Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 dump/content.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/dump/content.c b/dump/content.c
index 30232d4..c11d9b4 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -1382,17 +1382,10 @@ baseuuidbypass:
 	}
 
 	/* figure out the ino for the root directory of the fs
-	 * and get its struct xfs_bstat for inomap_build().  This could
-	 * be a bind mount; don't ask for the mount point inode,
-	 * find the actual lowest inode number in the filesystem.
+	 * and get its xfs_bstat_t for inomap_build()
 	 */
 	{
 		stat64_t rootstat;
-		xfs_ino_t lastino = 0;
-		int ocount = 0;
-		struct xfs_fsop_bulkreq bulkreq;
-
-		/* Get the inode of the mount point */
 		rval = fstat64(sc_fsfd, &rootstat);
 		if (rval) {
 			mlog(MLOG_NORMAL, _(
@@ -1404,21 +1397,11 @@ baseuuidbypass:
 			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
 		assert(sc_rootxfsstatp);
 
-		/* Get the first valid (i.e. root) inode in this fs */
-		bulkreq.lastip = (__u64 *)&lastino;
-		bulkreq.icount = 1;
-		bulkreq.ubuffer = sc_rootxfsstatp;
-		bulkreq.ocount = &ocount;
-		if (ioctl(sc_fsfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) {
+		if (bigstat_one(sc_fsfd, rootstat.st_ino, sc_rootxfsstatp) < 0) {
 			mlog(MLOG_ERROR,
 			      _("failed to get bulkstat information for root inode\n"));
 			return BOOL_FALSE;
 		}
-
-		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
-			mlog (MLOG_NORMAL | MLOG_NOTE,
-			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
-			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
 	}
 
 	/* alloc a file system handle, to be used with the jdm_open()
-- 
2.18.1

[PATCH v2 2/2] xfsdump: intercept bind mount targets

[Gao Xiang]

It's a bit strange pointing at some non-root bind mount target and
then actually dumping from the actual root dir instead.

Therefore, instead of searching for the root dir of the filesystem,
just intercept all bind mount targets by checking whose ino # of
".." is itself with getdents.

Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v1:
 just intercept bind mount targets suggested by Eric on IRC instead,
 and no need to use XFS_BULK_IREQ_SPECIAL_ROOT since the xfs header
 files and the kernel on my laptop don't support it, plus
 xfsdump/xfsrestore are not part of xfsprogs, so a bit hard to
 sync and make full use of that elegantly.

 dump/content.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/dump/content.c b/dump/content.c
index c11d9b4..7a55b92 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -511,6 +511,61 @@ static bool_t create_inv_session(
 		ix_t subtreecnt,
 		size_t strmix);
 
+static bool_t
+check_rootdir(int fd,
+	      xfs_ino_t ino)
+{
+	struct dirent	*gdp;
+	size_t		gdsz;
+
+	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
+	if (gdsz < GETDENTSBUF_SZ_MIN)
+		gdsz = GETDENTSBUF_SZ_MIN;
+	gdp = (struct dirent *)calloc(1, gdsz);
+	assert(gdp);
+
+	while (1) {
+		struct dirent *p;
+		int nread;
+
+		nread = getdents_wrap(fd, (char *)gdp, gdsz);
+		/*
+		 * negative count indicates something very bad happened;
+		 * try to gracefully end this dir.
+		 */
+		if (nread < 0) {
+			mlog(MLOG_NORMAL | MLOG_WARNING,
+_("unable to read dirents for directory ino %llu: %s\n"),
+			      ino, strerror(errno));
+			/* !!! curtis looked at this, and pointed out that
+			 * we could take some recovery action here. if the
+			 * errno is appropriate, lseek64 to the value of
+			 * doff field of the last dirent successfully
+			 * obtained, and contiue the loop.
+			 */
+			nread = 0; /* pretend we are done */
+		}
+
+		/* no more directory entries: break; */
+		if (!nread)
+			break;
+
+		for (p = gdp; nread > 0;
+		     nread -= (int)p->d_reclen,
+		     assert(nread >= 0),
+		     p = (struct dirent *)((char *)p + p->d_reclen)) {
+			if (!strcmp(p->d_name, "..") && p->d_ino == ino) {
+				mlog(MLOG_DEBUG, "FOUND: name %s d_ino %llu\n",
+				     p->d_name, ino);
+				free(gdp);
+				return BOOL_TRUE;
+			}
+		}
+	}
+	free(gdp);
+	return BOOL_FALSE;
+}
+
 bool_t
 content_init(int argc,
 	      char *argv[],
@@ -1393,6 +1448,13 @@ baseuuidbypass:
 			      mntpnt);
 			return BOOL_FALSE;
 		}
+
+		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
+			mlog(MLOG_ERROR,
+"oops, seems to be a bind mount, please use the actual mountpoint instead\n");
+			return BOOL_FALSE;
+		}
+
 		sc_rootxfsstatp =
 			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
 		assert(sc_rootxfsstatp);
-- 
2.18.1

Re: [PATCH v2 2/2] xfsdump: intercept bind mount targets

[Eric Sandeen]

On 11/2/20 8:33 PM, Gao Xiang wrote:
> It's a bit strange pointing at some non-root bind mount target and
> then actually dumping from the actual root dir instead.
> 
> Therefore, instead of searching for the root dir of the filesystem,
> just intercept all bind mount targets by checking whose ino # of
> ".." is itself with getdents.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v1:
>  just intercept bind mount targets suggested by Eric on IRC instead,
>  and no need to use XFS_BULK_IREQ_SPECIAL_ROOT since the xfs header
>  files and the kernel on my laptop don't support it, plus
>  xfsdump/xfsrestore are not part of xfsprogs, so a bit hard to
>  sync and make full use of that elegantly.

Thank you for doing this - it seems to be the least intrusive method to
finally fix this problem, I really appreciate you taking the work.

> 
>  dump/content.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/dump/content.c b/dump/content.c
> index c11d9b4..7a55b92 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -511,6 +511,61 @@ static bool_t create_inv_session(
>  		ix_t subtreecnt,
>  		size_t strmix);
>  
> +static bool_t
> +check_rootdir(int fd,
> +	      xfs_ino_t ino)
> +{
> +	struct dirent	*gdp;
> +	size_t		gdsz;
> +
> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
> +	if (gdsz < GETDENTSBUF_SZ_MIN)
> +		gdsz = GETDENTSBUF_SZ_MIN;
> +	gdp = (struct dirent *)calloc(1, gdsz);
> +	assert(gdp);
> +
> +	while (1) {
> +		struct dirent *p;
> +		int nread;
> +
> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> +		/*
> +		 * negative count indicates something very bad happened;
> +		 * try to gracefully end this dir.
> +		 */
> +		if (nread < 0) {
> +			mlog(MLOG_NORMAL | MLOG_WARNING,
> +_("unable to read dirents for directory ino %llu: %s\n"),
> +			      ino, strerror(errno));
> +			/* !!! curtis looked at this, and pointed out that

Nobody knows who curtis is, I think we can drop this comment now ;)
If we can't read the directory I think it's fine to simply error out here.

> +			 * we could take some recovery action here. if the
> +			 * errno is appropriate, lseek64 to the value of
> +			 * doff field of the last dirent successfully
> +			 * obtained, and contiue the loop.
> +			 */
> +			nread = 0; /* pretend we are done */
> +		}
> +
> +		/* no more directory entries: break; */
> +		if (!nread)
> +			break;
> +
> +		for (p = gdp; nread > 0;
> +		     nread -= (int)p->d_reclen,
> +		     assert(nread >= 0),
> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> +			if (!strcmp(p->d_name, "..") && p->d_ino == ino) {
> +				mlog(MLOG_DEBUG, "FOUND: name %s d_ino %llu\n",
> +				     p->d_name, ino);
> +				free(gdp);
> +				return BOOL_TRUE;
> +			}

I think we can stop as soon as we have found ".." yes?  No need to continue
iterating the directory, either ".." is what we wanted, or it's not, but either
way we are done when we have checked it.  On the off chance that we have
a very large root dir, stopping early might be good.

> +		}
> +	}
> +	free(gdp);
> +	return BOOL_FALSE;
> +}
> +
>  bool_t
>  content_init(int argc,
>  	      char *argv[],
> @@ -1393,6 +1448,13 @@ baseuuidbypass:
>  			      mntpnt);
>  			return BOOL_FALSE;
>  		}
> +
> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> +			mlog(MLOG_ERROR,
> +"oops, seems to be a bind mount, please use the actual mountpoint instead\n");

Could there be any other reason for this failure?  Maybe something like:

			mlog(MLOG_ERROR,
_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
				mntpnt);

or similar?

in any case I think it needs the i18n _("...") treatment.

Thanks!

-Eric

> +			return BOOL_FALSE;
> +		}
> +
>  		sc_rootxfsstatp =
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
> 

Re: [PATCH v2 2/2] xfsdump: intercept bind mount targets

[Gao Xiang]

Hi Eric,

On Tue, Nov 03, 2020 at 09:03:48AM -0600, Eric Sandeen wrote:
> On 11/2/20 8:33 PM, Gao Xiang wrote:

...

> > +
> > +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> > +		/*
> > +		 * negative count indicates something very bad happened;
> > +		 * try to gracefully end this dir.
> > +		 */
> > +		if (nread < 0) {
> > +			mlog(MLOG_NORMAL | MLOG_WARNING,
> > +_("unable to read dirents for directory ino %llu: %s\n"),
> > +			      ino, strerror(errno));
> > +			/* !!! curtis looked at this, and pointed out that
> 
> Nobody knows who curtis is, I think we can drop this comment now ;)
> If we can't read the directory I think it's fine to simply error out here.

This was copied from dump_dir(), ok, I will error out this.

> 
> > +			 * we could take some recovery action here. if the
> > +			 * errno is appropriate, lseek64 to the value of
> > +			 * doff field of the last dirent successfully
> > +			 * obtained, and contiue the loop.
> > +			 */
> > +			nread = 0; /* pretend we are done */
> > +		}
> > +
> > +		/* no more directory entries: break; */
> > +		if (!nread)
> > +			break;
> > +
> > +		for (p = gdp; nread > 0;
> > +		     nread -= (int)p->d_reclen,
> > +		     assert(nread >= 0),
> > +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> > +			if (!strcmp(p->d_name, "..") && p->d_ino == ino) {
> > +				mlog(MLOG_DEBUG, "FOUND: name %s d_ino %llu\n",
> > +				     p->d_name, ino);
> > +				free(gdp);
> > +				return BOOL_TRUE;
> > +			}
> 
> I think we can stop as soon as we have found ".." yes?  No need to continue
> iterating the directory, either ".." is what we wanted, or it's not, but either
> way we are done when we have checked it.  On the off chance that we have
> a very large root dir, stopping early might be good.

Yes, that is correct.

> 
> > +		}
> > +	}
> > +	free(gdp);
> > +	return BOOL_FALSE;
> > +}
> > +
> >  bool_t
> >  content_init(int argc,
> >  	      char *argv[],
> > @@ -1393,6 +1448,13 @@ baseuuidbypass:
> >  			      mntpnt);
> >  			return BOOL_FALSE;
> >  		}
> > +
> > +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> > +			mlog(MLOG_ERROR,
> > +"oops, seems to be a bind mount, please use the actual mountpoint instead\n");
> 
> Could there be any other reason for this failure?  Maybe something like:
> 
> 			mlog(MLOG_ERROR,
> _("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
> 				mntpnt);
> 
> or similar?
> 
> in any case I think it needs the i18n _("...") treatment.

Ok, will quickly send the fixed version about this!


Thanks for your review!


Thanks,
Gao Xiang

> 
> Thanks!
> 
> -Eric
> 
> > +			return BOOL_FALSE;
> > +		}
> > +
> >  		sc_rootxfsstatp =
> >  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
> >  		assert(sc_rootxfsstatp);
> > 
> 

[PATCH v3 2/2] xfsdump: intercept bind mount targets

[Gao Xiang]

It's a bit strange pointing at some non-root bind mount target and
then actually dumping from the actual root dir instead.

Therefore, instead of searching for the root dir of the filesystem,
just intercept all bind mount targets by checking whose ino # of
".." is itself with getdents.

Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v2 (Eric):
 - error out the case where the directory cannot be read;
 - In any case, stop as soon as we have found "..";
 - update the mountpoint error message and use i18n instead;

 dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/dump/content.c b/dump/content.c
index c11d9b4..c248e74 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -511,6 +511,55 @@ static bool_t create_inv_session(
 		ix_t subtreecnt,
 		size_t strmix);
 
+static bool_t
+check_rootdir(int fd,
+	      xfs_ino_t ino)
+{
+	struct dirent	*gdp;
+	size_t		gdsz;
+	bool_t		found = BOOL_FALSE;
+
+	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
+	if (gdsz < GETDENTSBUF_SZ_MIN)
+		gdsz = GETDENTSBUF_SZ_MIN;
+	gdp = (struct dirent *)calloc(1, gdsz);
+	assert(gdp);
+
+	while (1) {
+		struct dirent *p;
+		int nread;
+
+		nread = getdents_wrap(fd, (char *)gdp, gdsz);
+		/*
+		 * negative count indicates something very bad happened;
+		 * try to gracefully end this dir.
+		 */
+		if (nread < 0) {
+			mlog(MLOG_NORMAL | MLOG_WARNING,
+_("unable to read dirents for directory ino %llu: %s\n"),
+			      ino, strerror(errno));
+			break;
+		}
+
+		/* no more directory entries: break; */
+		if (!nread)
+			break;
+
+		for (p = gdp; nread > 0;
+		     nread -= (int)p->d_reclen,
+		     assert(nread >= 0),
+		     p = (struct dirent *)((char *)p + p->d_reclen)) {
+			if (!strcmp(p->d_name, "..")) {
+				if (p->d_ino == ino)
+					found = BOOL_TRUE;
+				break;
+			}
+		}
+	}
+	free(gdp);
+	return found;
+}
+
 bool_t
 content_init(int argc,
 	      char *argv[],
@@ -1393,6 +1442,14 @@ baseuuidbypass:
 			      mntpnt);
 			return BOOL_FALSE;
 		}
+
+		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
+			mlog(MLOG_ERROR,
+_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
+			     mntpnt);
+			return BOOL_FALSE;
+		}
+
 		sc_rootxfsstatp =
 			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
 		assert(sc_rootxfsstatp);
-- 
2.18.1

Re: [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets"

[Eric Sandeen]

On 11/2/20 8:33 PM, Gao Xiang wrote:
> Bind mount mntpnts will be forbided in the next commits
> instead since it's not the real rootdir.
> 
> This cannot be reverted cleanly due to several cleanup
> patches, but the logic is reverted equivalently.
> 
> This reverts commit 25195ebf107dc81b1b7cea1476764950e1d6cc9d.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Sorry this is so ridiculously late:

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

> ---
>  dump/content.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/dump/content.c b/dump/content.c
> index 30232d4..c11d9b4 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -1382,17 +1382,10 @@ baseuuidbypass:
>  	}
>  
>  	/* figure out the ino for the root directory of the fs
> -	 * and get its struct xfs_bstat for inomap_build().  This could
> -	 * be a bind mount; don't ask for the mount point inode,
> -	 * find the actual lowest inode number in the filesystem.
> +	 * and get its xfs_bstat_t for inomap_build()
>  	 */
>  	{
>  		stat64_t rootstat;
> -		xfs_ino_t lastino = 0;
> -		int ocount = 0;
> -		struct xfs_fsop_bulkreq bulkreq;
> -
> -		/* Get the inode of the mount point */
>  		rval = fstat64(sc_fsfd, &rootstat);
>  		if (rval) {
>  			mlog(MLOG_NORMAL, _(
> @@ -1404,21 +1397,11 @@ baseuuidbypass:
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
>  
> -		/* Get the first valid (i.e. root) inode in this fs */
> -		bulkreq.lastip = (__u64 *)&lastino;
> -		bulkreq.icount = 1;
> -		bulkreq.ubuffer = sc_rootxfsstatp;
> -		bulkreq.ocount = &ocount;
> -		if (ioctl(sc_fsfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) {
> +		if (bigstat_one(sc_fsfd, rootstat.st_ino, sc_rootxfsstatp) < 0) {
>  			mlog(MLOG_ERROR,
>  			      _("failed to get bulkstat information for root inode\n"));
>  			return BOOL_FALSE;
>  		}
> -
> -		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
> -			mlog (MLOG_NORMAL | MLOG_NOTE,
> -			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
> -			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
>  	}
>  
>  	/* alloc a file system handle, to be used with the jdm_open()
> 

Re: [PATCH v3 2/2] xfsdump: intercept bind mount targets

[Eric Sandeen]

On 11/3/20 9:33 AM, Gao Xiang wrote:
> It's a bit strange pointing at some non-root bind mount target and
> then actually dumping from the actual root dir instead.
> 
> Therefore, instead of searching for the root dir of the filesystem,
> just intercept all bind mount targets by checking whose ino # of
> ".." is itself with getdents.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v2 (Eric):
>  - error out the case where the directory cannot be read;
>  - In any case, stop as soon as we have found "..";
>  - update the mountpoint error message and use i18n instead;

This looks fine, thank you. I wish I'd done it properly the first time,
I'm glad you saw the proper way to do it.

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

>  dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/dump/content.c b/dump/content.c
> index c11d9b4..c248e74 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -511,6 +511,55 @@ static bool_t create_inv_session(
>  		ix_t subtreecnt,
>  		size_t strmix);
>  
> +static bool_t
> +check_rootdir(int fd,
> +	      xfs_ino_t ino)
> +{
> +	struct dirent	*gdp;
> +	size_t		gdsz;
> +	bool_t		found = BOOL_FALSE;
> +
> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
> +	if (gdsz < GETDENTSBUF_SZ_MIN)
> +		gdsz = GETDENTSBUF_SZ_MIN;
> +	gdp = (struct dirent *)calloc(1, gdsz);
> +	assert(gdp);
> +
> +	while (1) {
> +		struct dirent *p;
> +		int nread;
> +
> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> +		/*
> +		 * negative count indicates something very bad happened;
> +		 * try to gracefully end this dir.
> +		 */
> +		if (nread < 0) {
> +			mlog(MLOG_NORMAL | MLOG_WARNING,
> +_("unable to read dirents for directory ino %llu: %s\n"),
> +			      ino, strerror(errno));
> +			break;
> +		}
> +
> +		/* no more directory entries: break; */
> +		if (!nread)
> +			break;
> +
> +		for (p = gdp; nread > 0;
> +		     nread -= (int)p->d_reclen,
> +		     assert(nread >= 0),
> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> +			if (!strcmp(p->d_name, "..")) {
> +				if (p->d_ino == ino)
> +					found = BOOL_TRUE;
> +				break;
> +			}
> +		}
> +	}
> +	free(gdp);
> +	return found;
> +}
> +
>  bool_t
>  content_init(int argc,
>  	      char *argv[],
> @@ -1393,6 +1442,14 @@ baseuuidbypass:
>  			      mntpnt);
>  			return BOOL_FALSE;
>  		}
> +
> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> +			mlog(MLOG_ERROR,
> +_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
> +			     mntpnt);
> +			return BOOL_FALSE;
> +		}
> +
>  		sc_rootxfsstatp =
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
> 

Re: [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets"

[Bill O'Donnell]

On Tue, Nov 03, 2020 at 10:33:14AM +0800, Gao Xiang wrote:
> Bind mount mntpnts will be forbided in the next commits
> instead since it's not the real rootdir.
> 
> This cannot be reverted cleanly due to several cleanup
> patches, but the logic is reverted equivalently.
> 
> This reverts commit 25195ebf107dc81b1b7cea1476764950e1d6cc9d.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  dump/content.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/dump/content.c b/dump/content.c
> index 30232d4..c11d9b4 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -1382,17 +1382,10 @@ baseuuidbypass:
>  	}
>  
>  	/* figure out the ino for the root directory of the fs
> -	 * and get its struct xfs_bstat for inomap_build().  This could
> -	 * be a bind mount; don't ask for the mount point inode,
> -	 * find the actual lowest inode number in the filesystem.
> +	 * and get its xfs_bstat_t for inomap_build()
>  	 */
>  	{
>  		stat64_t rootstat;
> -		xfs_ino_t lastino = 0;
> -		int ocount = 0;
> -		struct xfs_fsop_bulkreq bulkreq;
> -
> -		/* Get the inode of the mount point */
>  		rval = fstat64(sc_fsfd, &rootstat);
>  		if (rval) {
>  			mlog(MLOG_NORMAL, _(
> @@ -1404,21 +1397,11 @@ baseuuidbypass:
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
>  
> -		/* Get the first valid (i.e. root) inode in this fs */
> -		bulkreq.lastip = (__u64 *)&lastino;
> -		bulkreq.icount = 1;
> -		bulkreq.ubuffer = sc_rootxfsstatp;
> -		bulkreq.ocount = &ocount;
> -		if (ioctl(sc_fsfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) {
> +		if (bigstat_one(sc_fsfd, rootstat.st_ino, sc_rootxfsstatp) < 0) {
>  			mlog(MLOG_ERROR,
>  			      _("failed to get bulkstat information for root inode\n"));
>  			return BOOL_FALSE;
>  		}
> -
> -		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
> -			mlog (MLOG_NORMAL | MLOG_NOTE,
> -			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
> -			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
>  	}
>  
>  	/* alloc a file system handle, to be used with the jdm_open()
> -- 
> 2.18.1
> 

Re: [PATCH v3 2/2] xfsdump: intercept bind mount targets

[Bill O'Donnell]

On Tue, Nov 03, 2020 at 11:33:28PM +0800, Gao Xiang wrote:
> It's a bit strange pointing at some non-root bind mount target and
> then actually dumping from the actual root dir instead.
> 
> Therefore, instead of searching for the root dir of the filesystem,
> just intercept all bind mount targets by checking whose ino # of
> ".." is itself with getdents.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
> changes since v2 (Eric):
>  - error out the case where the directory cannot be read;
>  - In any case, stop as soon as we have found "..";
>  - update the mountpoint error message and use i18n instead;
> 
>  dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/dump/content.c b/dump/content.c
> index c11d9b4..c248e74 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -511,6 +511,55 @@ static bool_t create_inv_session(
>  		ix_t subtreecnt,
>  		size_t strmix);
>  
> +static bool_t
> +check_rootdir(int fd,
> +	      xfs_ino_t ino)
> +{
> +	struct dirent	*gdp;
> +	size_t		gdsz;
> +	bool_t		found = BOOL_FALSE;
> +
> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
> +	if (gdsz < GETDENTSBUF_SZ_MIN)
> +		gdsz = GETDENTSBUF_SZ_MIN;
> +	gdp = (struct dirent *)calloc(1, gdsz);
> +	assert(gdp);
> +
> +	while (1) {
> +		struct dirent *p;
> +		int nread;
> +
> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> +		/*
> +		 * negative count indicates something very bad happened;
> +		 * try to gracefully end this dir.
> +		 */
> +		if (nread < 0) {
> +			mlog(MLOG_NORMAL | MLOG_WARNING,
> +_("unable to read dirents for directory ino %llu: %s\n"),
> +			      ino, strerror(errno));
> +			break;
> +		}
> +
> +		/* no more directory entries: break; */
> +		if (!nread)
> +			break;
> +
> +		for (p = gdp; nread > 0;
> +		     nread -= (int)p->d_reclen,
> +		     assert(nread >= 0),
> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> +			if (!strcmp(p->d_name, "..")) {
> +				if (p->d_ino == ino)
> +					found = BOOL_TRUE;
> +				break;
> +			}
> +		}
> +	}
> +	free(gdp);
> +	return found;
> +}
> +
>  bool_t
>  content_init(int argc,
>  	      char *argv[],
> @@ -1393,6 +1442,14 @@ baseuuidbypass:
>  			      mntpnt);
>  			return BOOL_FALSE;
>  		}
> +
> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> +			mlog(MLOG_ERROR,
> +_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
> +			     mntpnt);
> +			return BOOL_FALSE;
> +		}
> +
>  		sc_rootxfsstatp =
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
> -- 
> 2.18.1
> 

Re: [PATCH v3 2/2] xfsdump: intercept bind mount targets

[Masayoshi Mizuma]

On Tue, Nov 03, 2020 at 11:33:28PM +0800, Gao Xiang wrote:
> It's a bit strange pointing at some non-root bind mount target and
> then actually dumping from the actual root dir instead.
> 
> Therefore, instead of searching for the root dir of the filesystem,
> just intercept all bind mount targets by checking whose ino # of
> ".." is itself with getdents.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Hi,

This patch works for the filesystem which the inode number of the
root directory is different from the root inode number of the
filesystem without the bind mount.

Please feel free to add:

  Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

The log with the both of the patch:

  # xfs_io -i -c 'bulkstat_single root' /test/MNT1 | awk '/bs_ino = /{print $3}'
  128
  # stat --printf="%i\n" /test/MNT1
  1024
  # xfsdump -L session -M test -f /tmp/2176346.dump /test/MNT1
  xfsdump: using file dump (drive_simple) strategy
  xfsdump: version 3.1.9 (dump format 3.0) - type ^C for status and control
  xfsdump: level 0 dump of localhost:/test/MNT1
  xfsdump: dump date: Mon Dec 20 10:38:27 2021
  xfsdump: session id: edec5c99-062b-41b8-a0c2-6c3c87d7ce75
  xfsdump: session label: "session"
  xfsdump: ino map phase 1: constructing initial dump list
  xfsdump: ino map phase 2: skipping (no pruning necessary)
  xfsdump: ino map phase 3: skipping (only one dump stream)
  xfsdump: ino map construction complete
  xfsdump: estimated dump size: 532800 bytes
  xfsdump: /var/lib/xfsdump/inventory created
  xfsdump: creating dump session media file 0 (media 0, file 0)
  xfsdump: dumping ino map
  xfsdump: dumping directories
  xfsdump: dumping non-directory files
  xfsdump: ending media file
  xfsdump: media file size 1061824 bytes
  xfsdump: dump size (non-dir files) : 0 bytes
  xfsdump: dump complete: 3 seconds elapsed
  xfsdump: Dump Summary:
  xfsdump:   stream 0 /tmp/2176346.dump OK (success)
  xfsdump: Dump Status: SUCCESS

Thanks!
Masa

> ---
> changes since v2 (Eric):
>  - error out the case where the directory cannot be read;
>  - In any case, stop as soon as we have found "..";
>  - update the mountpoint error message and use i18n instead;
> 
>  dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/dump/content.c b/dump/content.c
> index c11d9b4..c248e74 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -511,6 +511,55 @@ static bool_t create_inv_session(
>  		ix_t subtreecnt,
>  		size_t strmix);
>  
> +static bool_t
> +check_rootdir(int fd,
> +	      xfs_ino_t ino)
> +{
> +	struct dirent	*gdp;
> +	size_t		gdsz;
> +	bool_t		found = BOOL_FALSE;
> +
> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
> +	if (gdsz < GETDENTSBUF_SZ_MIN)
> +		gdsz = GETDENTSBUF_SZ_MIN;
> +	gdp = (struct dirent *)calloc(1, gdsz);
> +	assert(gdp);
> +
> +	while (1) {
> +		struct dirent *p;
> +		int nread;
> +
> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> +		/*
> +		 * negative count indicates something very bad happened;
> +		 * try to gracefully end this dir.
> +		 */
> +		if (nread < 0) {
> +			mlog(MLOG_NORMAL | MLOG_WARNING,
> +_("unable to read dirents for directory ino %llu: %s\n"),
> +			      ino, strerror(errno));
> +			break;
> +		}
> +
> +		/* no more directory entries: break; */
> +		if (!nread)
> +			break;
> +
> +		for (p = gdp; nread > 0;
> +		     nread -= (int)p->d_reclen,
> +		     assert(nread >= 0),
> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> +			if (!strcmp(p->d_name, "..")) {
> +				if (p->d_ino == ino)
> +					found = BOOL_TRUE;
> +				break;
> +			}
> +		}
> +	}
> +	free(gdp);
> +	return found;
> +}
> +
>  bool_t
>  content_init(int argc,
>  	      char *argv[],
> @@ -1393,6 +1442,14 @@ baseuuidbypass:
>  			      mntpnt);
>  			return BOOL_FALSE;
>  		}
> +
> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> +			mlog(MLOG_ERROR,
> +_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
> +			     mntpnt);
> +			return BOOL_FALSE;
> +		}
> +
>  		sc_rootxfsstatp =
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
> -- 
> 2.18.1
> 

Re: [PATCH v3 2/2] xfsdump: intercept bind mount targets

[Eric Sandeen]

On 12/20/21 10:01 AM, Masayoshi Mizuma wrote:
> On Tue, Nov 03, 2020 at 11:33:28PM +0800, Gao Xiang wrote:
>> It's a bit strange pointing at some non-root bind mount target and
>> then actually dumping from the actual root dir instead.
>>
>> Therefore, instead of searching for the root dir of the filesystem,
>> just intercept all bind mount targets by checking whose ino # of
>> ".." is itself with getdents.
>>
>> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
>> Cc: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> 
> Hi,
> 
> This patch works for the filesystem which the inode number of the
> root directory is different from the root inode number of the
> filesystem without the bind mount.
> 
> Please feel free to add:
> 
>    Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Great, thank you for the test results.

-Eric

> The log with the both of the patch:
> 
>    # xfs_io -i -c 'bulkstat_single root' /test/MNT1 | awk '/bs_ino = /{print $3}'
>    128
>    # stat --printf="%i\n" /test/MNT1
>    1024
>    # xfsdump -L session -M test -f /tmp/2176346.dump /test/MNT1
>    xfsdump: using file dump (drive_simple) strategy
>    xfsdump: version 3.1.9 (dump format 3.0) - type ^C for status and control
>    xfsdump: level 0 dump of localhost:/test/MNT1
>    xfsdump: dump date: Mon Dec 20 10:38:27 2021
>    xfsdump: session id: edec5c99-062b-41b8-a0c2-6c3c87d7ce75
>    xfsdump: session label: "session"
>    xfsdump: ino map phase 1: constructing initial dump list
>    xfsdump: ino map phase 2: skipping (no pruning necessary)
>    xfsdump: ino map phase 3: skipping (only one dump stream)
>    xfsdump: ino map construction complete
>    xfsdump: estimated dump size: 532800 bytes
>    xfsdump: /var/lib/xfsdump/inventory created
>    xfsdump: creating dump session media file 0 (media 0, file 0)
>    xfsdump: dumping ino map
>    xfsdump: dumping directories
>    xfsdump: dumping non-directory files
>    xfsdump: ending media file
>    xfsdump: media file size 1061824 bytes
>    xfsdump: dump size (non-dir files) : 0 bytes
>    xfsdump: dump complete: 3 seconds elapsed
>    xfsdump: Dump Summary:
>    xfsdump:   stream 0 /tmp/2176346.dump OK (success)
>    xfsdump: Dump Status: SUCCESS
> 
> Thanks!
> Masa
> 
>> ---
>> changes since v2 (Eric):
>>   - error out the case where the directory cannot be read;
>>   - In any case, stop as soon as we have found "..";
>>   - update the mountpoint error message and use i18n instead;
>>
>>   dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/dump/content.c b/dump/content.c
>> index c11d9b4..c248e74 100644
>> --- a/dump/content.c
>> +++ b/dump/content.c
>> @@ -511,6 +511,55 @@ static bool_t create_inv_session(
>>   		ix_t subtreecnt,
>>   		size_t strmix);
>>   
>> +static bool_t
>> +check_rootdir(int fd,
>> +	      xfs_ino_t ino)
>> +{
>> +	struct dirent	*gdp;
>> +	size_t		gdsz;
>> +	bool_t		found = BOOL_FALSE;
>> +
>> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
>> +	if (gdsz < GETDENTSBUF_SZ_MIN)
>> +		gdsz = GETDENTSBUF_SZ_MIN;
>> +	gdp = (struct dirent *)calloc(1, gdsz);
>> +	assert(gdp);
>> +
>> +	while (1) {
>> +		struct dirent *p;
>> +		int nread;
>> +
>> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
>> +		/*
>> +		 * negative count indicates something very bad happened;
>> +		 * try to gracefully end this dir.
>> +		 */
>> +		if (nread < 0) {
>> +			mlog(MLOG_NORMAL | MLOG_WARNING,
>> +_("unable to read dirents for directory ino %llu: %s\n"),
>> +			      ino, strerror(errno));
>> +			break;
>> +		}
>> +
>> +		/* no more directory entries: break; */
>> +		if (!nread)
>> +			break;
>> +
>> +		for (p = gdp; nread > 0;
>> +		     nread -= (int)p->d_reclen,
>> +		     assert(nread >= 0),
>> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
>> +			if (!strcmp(p->d_name, "..")) {
>> +				if (p->d_ino == ino)
>> +					found = BOOL_TRUE;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +	free(gdp);
>> +	return found;
>> +}
>> +
>>   bool_t
>>   content_init(int argc,
>>   	      char *argv[],
>> @@ -1393,6 +1442,14 @@ baseuuidbypass:
>>   			      mntpnt);
>>   			return BOOL_FALSE;
>>   		}
>> +
>> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
>> +			mlog(MLOG_ERROR,
>> +_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
>> +			     mntpnt);
>> +			return BOOL_FALSE;
>> +		}
>> +
>>   		sc_rootxfsstatp =
>>   			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>>   		assert(sc_rootxfsstatp);
>> -- 
>> 2.18.1
>>
>