* [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE @ 2020-01-28 16:07 Jeff Moyer 2020-01-28 16:14 ` Darrick J. Wong 2021-04-16 17:39 ` Eric Sandeen 0 siblings, 2 replies; 6+ messages in thread From: Jeff Moyer @ 2020-01-28 16:07 UTC (permalink / raw) To: linux-xfs 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE 2020-01-28 16:07 [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE Jeff Moyer @ 2020-01-28 16:14 ` Darrick J. Wong 2020-01-28 16:54 ` Jeff Moyer 2020-02-04 18:38 ` Jeff Moyer 2021-04-16 17:39 ` Eric Sandeen 1 sibling, 2 replies; 6+ messages in thread From: Darrick J. Wong @ 2020-01-28 16:14 UTC (permalink / raw) To: Jeff Moyer; +Cc: linux-xfs 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE 2020-01-28 16:14 ` Darrick J. Wong @ 2020-01-28 16:54 ` Jeff Moyer 2020-01-28 23:53 ` Dave Chinner 2020-02-04 18:38 ` Jeff Moyer 1 sibling, 1 reply; 6+ messages in thread From: Jeff Moyer @ 2020-01-28 16:54 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE 2020-01-28 16:54 ` Jeff Moyer @ 2020-01-28 23:53 ` Dave Chinner 0 siblings, 0 replies; 6+ messages in thread From: Dave Chinner @ 2020-01-28 23:53 UTC (permalink / raw) To: Jeff Moyer; +Cc: Darrick J. Wong, linux-xfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE 2020-01-28 16:14 ` Darrick J. Wong 2020-01-28 16:54 ` Jeff Moyer @ 2020-02-04 18:38 ` Jeff Moyer 1 sibling, 0 replies; 6+ messages in thread From: Jeff Moyer @ 2020-02-04 18:38 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE 2020-01-28 16:07 [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE Jeff Moyer 2020-01-28 16:14 ` Darrick J. Wong @ 2021-04-16 17:39 ` Eric Sandeen 1 sibling, 0 replies; 6+ messages in thread From: Eric Sandeen @ 2021-04-16 17:39 UTC (permalink / raw) To: Jeff Moyer, linux-xfs 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-16 17:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-28 16:07 [PATCH] xfsprogs: mkfs: don't default to the physical sector size if it's bigger than XFS_MAX_SECTORSIZE Jeff Moyer 2020-01-28 16:14 ` Darrick J. Wong 2020-01-28 16:54 ` Jeff Moyer 2020-01-28 23:53 ` Dave Chinner 2020-02-04 18:38 ` Jeff Moyer 2021-04-16 17:39 ` Eric Sandeen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).