[PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE

[PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE

[Jeff Moyer]

Hi,

In testing on ppc64, I ran into the following error when making a file
system:

# ./mkfs.xfs -b size=65536 -f /dev/ram0
illegal sector size 65536

Which is odd, because I didn't specify a sector size!  The problem is
that validate_sectorsize defaults to the physical sector size, but in
this case, the physical sector size is bigger than XFS_MAX_SECTORSIZE.

# cat /sys/block/ram0/queue/physical_block_size 
65536

Fall back to the default (logical sector size) if the physical sector
size is greater than XFS_MAX_SECTORSIZE.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 606f79da..dc9858af 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1803,8 +1803,13 @@ validate_sectorsize(
 		if (!ft->lsectorsize)
 			ft->lsectorsize = XFS_MIN_SECTORSIZE;
 
-		/* Older kernels may not have physical/logical distinction */
-		if (!ft->psectorsize)
+		/*
+		 * Older kernels may not have physical/logical distinction.
+		 * Some architectures have a page size > XFS_MAX_SECTORSIZE.
+		 * In that case, a ramdisk or persistent memory device may
+		 * advertise a physical sector size that is too big to use.
+		 */
+		if (!ft->psectorsize || ft->psectorsize > XFS_MAX_SECTORSIZE)
 			ft->psectorsize = ft->lsectorsize;
 
 		cfg->sectorsize = ft->psectorsize;

Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE

[Darrick J. Wong]

On Tue, Jan 28, 2020 at 11:07:01AM -0500, Jeff Moyer wrote:
> Hi,
> 
> In testing on ppc64, I ran into the following error when making a file
> system:
> 
> # ./mkfs.xfs -b size=65536 -f /dev/ram0
> illegal sector size 65536
> 
> Which is odd, because I didn't specify a sector size!  The problem is
> that validate_sectorsize defaults to the physical sector size, but in
> this case, the physical sector size is bigger than XFS_MAX_SECTORSIZE.
> 
> # cat /sys/block/ram0/queue/physical_block_size 
> 65536
> 
> Fall back to the default (logical sector size) if the physical sector
> size is greater than XFS_MAX_SECTORSIZE.

Do we need to check that ft->lsectorsize <= XFS_MAX_SECTORSIZE too?

(Not that I really expect disks with 64k LBA units...)

--D

> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 606f79da..dc9858af 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1803,8 +1803,13 @@ validate_sectorsize(
>  		if (!ft->lsectorsize)
>  			ft->lsectorsize = XFS_MIN_SECTORSIZE;
>  
> -		/* Older kernels may not have physical/logical distinction */
> -		if (!ft->psectorsize)
> +		/*
> +		 * Older kernels may not have physical/logical distinction.
> +		 * Some architectures have a page size > XFS_MAX_SECTORSIZE.
> +		 * In that case, a ramdisk or persistent memory device may
> +		 * advertise a physical sector size that is too big to use.
> +		 */
> +		if (!ft->psectorsize || ft->psectorsize > XFS_MAX_SECTORSIZE)
>  			ft->psectorsize = ft->lsectorsize;
>  
>  		cfg->sectorsize = ft->psectorsize;
> 

Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE

[Jeff Moyer]

"Darrick J. Wong" <darrick.wong@oracle.com> writes:

> Do we need to check that ft->lsectorsize <= XFS_MAX_SECTORSIZE too?

We can.  What would be the correct response to such a situation?

> (Not that I really expect disks with 64k LBA units...)

It looks like you can tell loop to do that, but I wasn't able to make
that happen in practice.  I'm not quite sure why, though.

-Jeff

Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE

[Dave Chinner]

On Tue, Jan 28, 2020 at 11:54:17AM -0500, Jeff Moyer wrote:
> "Darrick J. Wong" <darrick.wong@oracle.com> writes:
> 
> > Do we need to check that ft->lsectorsize <= XFS_MAX_SECTORSIZE too?
> 
> We can.  What would be the correct response to such a situation?

Fail. The device is outside of the bounds of the validated on-disk
format at that point.

Historically speaking, the on-disk format only supports up to 32kB
sector sizes and the limitation is a journal format limitation. That
is, the journal implementation is based around 32kB buffers and
journal writes requiring atomic sector writes. Even though v2 logs
can support up to 256kB buffers, the format is still based around
32kB "segments" within the larger buffer.

IIRC, this limitation is due to the built-in sector-based "whole
buffer written" validation mechanism built into the log headers.
The v1 format has an array that can store 64 individual sector
indexes, so for 512 byte sectors the max log buffer size is 32kB.
As long as that first sector of the log buffer is written
atomically, it doesn't matter what the sector size actually is.

However, we cannot do 32kB writes to a 64kB physical sector device,
and hence the v1 format icannot support log records larger than
32kB, so the format is limited to 32kB sector sizes.

The v2 log format does increase the size of the log buffer, but it
does so by adding extension header sectors that contain the index
mapping for each 32kB sector. It works under the same principle as
the v1 log - as long as the first sector write is atomic, we can
validate the extension headers are intact and so validate all 32kB
segments of the larger buffer.

The v2 log format also allows for stripe unit padding of writes,
such that it will always write entire stripe units even when the log
buffer is not full. Hence it supports writing aligned sizes larger
than 32kB, even up to 256kB. However, it still relies on atomic
sector writes for validation.

When we moved to the v5 on-disk format, we also turned on CRC
checking of the log records. THis means we are no longer dependent
on atomic sector writes to detect torn log writes - if the first
512 byte sector is not written atomically, we can detect that in
recovery on v5 filesystems. That means we don't really care what the
sector size used by the log is anymore. We can do aligned log writes
up to 256kB, and we can detect torn single sector writes.

Hence I suspect XFS_MAX_SECTORSIZE is no longer relevant to v5
filesystems and we may be able to increase it to 64kB without any
issues. We'd need to do quite a bit more code spelunking than I've
just done, not to mention validation testing, before we can do such
a thing, though....

> > (Not that I really expect disks with 64k LBA units...)
> 
> It looks like you can tell loop to do that, but I wasn't able to make
> that happen in practice.  I'm not quite sure why, though.

IIRC, it depends on what IO alignment capability the underlying
filesystem provides via it's sector/block sizes.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE

[Jeff Moyer]

"Darrick J. Wong" <darrick.wong@oracle.com> writes:

> On Tue, Jan 28, 2020 at 11:07:01AM -0500, Jeff Moyer wrote:
>> Hi,
>> 
>> In testing on ppc64, I ran into the following error when making a file
>> system:
>> 
>> # ./mkfs.xfs -b size=65536 -f /dev/ram0
>> illegal sector size 65536
>> 
>> Which is odd, because I didn't specify a sector size!  The problem is
>> that validate_sectorsize defaults to the physical sector size, but in
>> this case, the physical sector size is bigger than XFS_MAX_SECTORSIZE.
>> 
>> # cat /sys/block/ram0/queue/physical_block_size 
>> 65536
>> 
>> Fall back to the default (logical sector size) if the physical sector
>> size is greater than XFS_MAX_SECTORSIZE.
>
> Do we need to check that ft->lsectorsize <= XFS_MAX_SECTORSIZE too?

Actually, that's done later in the same function:

        /* validate specified/probed sector size */
        if (cfg->sectorsize < XFS_MIN_SECTORSIZE ||
            cfg->sectorsize > XFS_MAX_SECTORSIZE) {
                fprintf(stderr, _("illegal sector size %d\n"), cfg->sectorsize);
                usage();
        }

-Jeff

Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE

[Eric Sandeen]

On 1/28/20 10:07 AM, Jeff Moyer wrote:
> Hi,
> 
> In testing on ppc64, I ran into the following error when making a file
> system:
> 
> # ./mkfs.xfs -b size=65536 -f /dev/ram0
> illegal sector size 65536
> 
> Which is odd, because I didn't specify a sector size!  The problem is
> that validate_sectorsize defaults to the physical sector size, but in
> this case, the physical sector size is bigger than XFS_MAX_SECTORSIZE.
> 
> # cat /sys/block/ram0/queue/physical_block_size 
> 65536
> 
> Fall back to the default (logical sector size) if the physical sector
> size is greater than XFS_MAX_SECTORSIZE.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Looks fine to me, sorry for the long delay; I think you answered the
question about checking the logical sector size (we validate it
later on)

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

> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 606f79da..dc9858af 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1803,8 +1803,13 @@ validate_sectorsize(
>  		if (!ft->lsectorsize)
>  			ft->lsectorsize = XFS_MIN_SECTORSIZE;
>  
> -		/* Older kernels may not have physical/logical distinction */
> -		if (!ft->psectorsize)
> +		/*
> +		 * Older kernels may not have physical/logical distinction.
> +		 * Some architectures have a page size > XFS_MAX_SECTORSIZE.
> +		 * In that case, a ramdisk or persistent memory device may
> +		 * advertise a physical sector size that is too big to use.
> +		 */
> +		if (!ft->psectorsize || ft->psectorsize > XFS_MAX_SECTORSIZE)
>  			ft->psectorsize = ft->lsectorsize;
>  
>  		cfg->sectorsize = ft->psectorsize;
>