| Message ID | 20240112044743.2254211-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] libxfs: remove the unused fs_topology_t typedef | expand |
On Fri, Jan 12, 2024 at 05:47:41AM +0100, Christoph Hellwig wrote: > fs_topology is a mess that mixes up data and RT device reporting, > and to make things worse reuses lsectorsize for the logical sector > size while other parts of xfsprogs use it for the log sector size. Gaaaaah. > Split out a device_topology structure that reports the topology for > one device and embedded two of them into the fs_topology struture. > Rename the sector size members to be more explicit, and move some > of the sanity checking from mkfs into the topology helpers. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > libxfs/topology.c | 61 ++++++++++++++++++++++++++++---------------- > libxfs/topology.h | 14 +++++++---- > mkfs/xfs_mkfs.c | 64 +++++++++++++++++++++-------------------------- > repair/sb.c | 2 +- > 4 files changed, 78 insertions(+), 63 deletions(-) > > diff --git a/libxfs/topology.c b/libxfs/topology.c > index 06013d429..227f8c44f 100644 > --- a/libxfs/topology.c > +++ b/libxfs/topology.c > @@ -286,47 +286,64 @@ static void blkid_get_topology( > > #endif /* ENABLE_BLKID */ > > -void > -get_topology( > - struct libxfs_init *xi, > - struct fs_topology *ft, > +static void > +get_device_topology( > + struct libxfs_dev *dev, > + struct device_topology *dt, > int force_overwrite) > { > - struct stat statbuf; > + struct stat st; > + > + /* > + * Nothing to do if this particular subvolume doesn't exist. > + */ > + if (!dev->name) > + return; > > /* > * If our target is a regular file, use platform_findsizes > * to try to obtain the underlying filesystem's requirements > * for direct IO; we'll set our sector size to that if possible. > */ > - if (xi->data.isfile || > - (!stat(xi->data.name, &statbuf) && S_ISREG(statbuf.st_mode))) { > - int fd; > + if (dev->isfile || (!stat(dev->name, &st) && S_ISREG(st.st_mode))) { > int flags = O_RDONLY; > long long dummy; > + int fd; > > /* with xi->disfile we may not have the file yet! */ > - if (xi->data.isfile) > + if (dev->isfile) > flags |= O_CREAT; > > - fd = open(xi->data.name, flags, 0666); > + fd = open(dev->name, flags, 0666); > if (fd >= 0) { > - platform_findsizes(xi->data.name, fd, &dummy, > - &ft->lsectorsize); > + platform_findsizes(dev->name, fd, &dummy, > + &dt->logical_sector_size); > close(fd); > - ft->psectorsize = ft->lsectorsize; > - } else > - ft->psectorsize = ft->lsectorsize = BBSIZE; > + } else { > + dt->logical_sector_size = BBSIZE; > + } > } else { > - blkid_get_topology(xi->data.name, &ft->dsunit, &ft->dswidth, > - &ft->lsectorsize, &ft->psectorsize, > + blkid_get_topology(dev->name, &dt->sunit, &dt->swidth, > + &dt->logical_sector_size, > + &dt->physical_sector_size, > force_overwrite); > } > > - if (xi->rt.name && !xi->rt.isfile) { > - int sunit, lsectorsize, psectorsize; > + ASSERT(dt->logical_sector_size); > > - blkid_get_topology(xi->rt.name, &sunit, &ft->rtswidth, > - &lsectorsize, &psectorsize, force_overwrite); > - } > + /* > + * Older kernels may not have physical/logical distinction. > + */ > + if (!dt->physical_sector_size) > + dt->physical_sector_size = dt->logical_sector_size; > +} > + > +void > +get_topology( > + struct libxfs_init *xi, > + struct fs_topology *ft, > + int force_overwrite) > +{ > + get_device_topology(&xi->data, &ft->data, force_overwrite); > + get_device_topology(&xi->rt, &ft->rt, force_overwrite); > } > diff --git a/libxfs/topology.h b/libxfs/topology.h > index 3a309a4da..ba0c8f669 100644 > --- a/libxfs/topology.h > +++ b/libxfs/topology.h > @@ -10,12 +10,16 @@ > /* > * Device topology information. > */ > +struct device_topology { > + int logical_sector_size; /* logical sector size */ > + int physical_sector_size; /* physical sector size */ > + int sunit; /* stripe unit */ > + int swidth; /* stripe width */ I've long wondered if we should clean up the signed/unsigned int/long mixing that goes on in the topology code. The blkid functions return unsigned longs, but eventually we mash them into signed ints here. Obviously this is a topic for a separate patch. > +}; > + > struct fs_topology { > - int dsunit; /* stripe unit - data subvolume */ > - int dswidth; /* stripe width - data subvolume */ > - int rtswidth; /* stripe width - rt subvolume */ > - int lsectorsize; /* logical sector size &*/ > - int psectorsize; /* physical sector size */ > + struct device_topology data; > + struct device_topology rt; Neat! The conversion logic looks straightforward, so Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > }; > > void > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index fcbf54132..be65ccc1e 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -1986,31 +1986,24 @@ validate_sectorsize( > * than that, then we can use logical, but warn about the > * inefficiency. > * > - * Set the topology sectors if they were not probed to the > - * minimum supported sector size. > - */ > - if (!ft->lsectorsize) > - ft->lsectorsize = dft->sectorsize; > - > - /* > - * 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; > + if (ft->data.physical_sector_size > XFS_MAX_SECTORSIZE) { > + ft->data.physical_sector_size = > + ft->data.logical_sector_size; > + } > > - cfg->sectorsize = ft->psectorsize; > + cfg->sectorsize = ft->data.physical_sector_size; > if (cfg->blocksize < cfg->sectorsize && > - cfg->blocksize >= ft->lsectorsize) { > + cfg->blocksize >= ft->data.logical_sector_size) { > fprintf(stderr, > _("specified blocksize %d is less than device physical sector size %d\n" > "switching to logical sector size %d\n"), > - cfg->blocksize, ft->psectorsize, > - ft->lsectorsize); > - cfg->sectorsize = ft->lsectorsize; > + cfg->blocksize, ft->data.physical_sector_size, > + ft->data.logical_sector_size); > + cfg->sectorsize = ft->data.logical_sector_size; > } > } else > cfg->sectorsize = cli->sectorsize; > @@ -2031,9 +2024,9 @@ _("block size %d cannot be smaller than sector size %d\n"), > usage(); > } > > - if (cfg->sectorsize < ft->lsectorsize) { > + if (cfg->sectorsize < ft->data.logical_sector_size) { > fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"), > - cfg->sectorsize, ft->lsectorsize); > + cfg->sectorsize, ft->data.logical_sector_size); > usage(); > } > } > @@ -2455,7 +2448,7 @@ validate_rtextsize( > > if (!cfg->sb_feat.nortalign && !cli->xi->rt.isfile && > !(!cli->rtsize && cli->xi->data.isfile)) > - rswidth = ft->rtswidth; > + rswidth = ft->rt.swidth; > else > rswidth = 0; > > @@ -2700,13 +2693,14 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( > /* if no stripe config set, use the device default */ > if (!dsunit) { > /* Ignore nonsense from device report. */ > - if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit), > - BBTOB(ft->dswidth), 0, true)) { > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->data.sunit), > + BBTOB(ft->data.swidth), 0, true)) { > fprintf(stderr, > _("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), > - progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); > - ft->dsunit = 0; > - ft->dswidth = 0; > + progname, > + BBTOB(ft->data.sunit), BBTOB(ft->data.swidth)); > + ft->data.sunit = 0; > + ft->data.swidth = 0; > } else if (cfg->dblocks < GIGABYTES(1, cfg->blocklog)) { > /* > * Don't use automatic stripe detection if the device > @@ -2714,29 +2708,29 @@ _("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\ > * on such a small system are not worth the risk that > * we'll end up with an undersized log. > */ > - if (ft->dsunit || ft->dswidth) > + if (ft->data.sunit || ft->data.swidth) > fprintf(stderr, > _("%s: small data volume, ignoring data volume stripe unit %d and stripe width %d\n"), > - progname, ft->dsunit, > - ft->dswidth); > - ft->dsunit = 0; > - ft->dswidth = 0; > + progname, ft->data.sunit, > + ft->data.swidth); > + ft->data.sunit = 0; > + ft->data.swidth = 0; > } else { > - dsunit = ft->dsunit; > - dswidth = ft->dswidth; > + dsunit = ft->data.sunit; > + dswidth = ft->data.swidth; > use_dev = true; > } > } else { > /* check and warn if user-specified alignment is sub-optimal */ > - if (ft->dsunit && ft->dsunit != dsunit) { > + if (ft->data.sunit && ft->data.sunit != dsunit) { > fprintf(stderr, > _("%s: Specified data stripe unit %d is not the same as the volume stripe unit %d\n"), > - progname, dsunit, ft->dsunit); > + progname, dsunit, ft->data.sunit); > } > - if (ft->dswidth && ft->dswidth != dswidth) { > + if (ft->data.swidth && ft->data.swidth != dswidth) { > fprintf(stderr, > _("%s: Specified data stripe width %d is not the same as the volume stripe width %d\n"), > - progname, dswidth, ft->dswidth); > + progname, dswidth, ft->data.swidth); > } > } > > diff --git a/repair/sb.c b/repair/sb.c > index dedac53af..02c10d9a5 100644 > --- a/repair/sb.c > +++ b/repair/sb.c > @@ -189,7 +189,7 @@ guess_default_geometry( > * Use default block size (2^12) > */ > blocklog = 12; > - multidisk = ft.dswidth | ft.dsunit; > + multidisk = ft.data.swidth | ft.data.sunit; > dblocks = x->data.size >> (blocklog - BBSHIFT); > calc_default_ag_geometry(blocklog, dblocks, multidisk, > agsize, agcount); > -- > 2.39.2 > >
diff --git a/libxfs/topology.c b/libxfs/topology.c index 06013d429..227f8c44f 100644 --- a/libxfs/topology.c +++ b/libxfs/topology.c @@ -286,47 +286,64 @@ static void blkid_get_topology( #endif /* ENABLE_BLKID */ -void -get_topology( - struct libxfs_init *xi, - struct fs_topology *ft, +static void +get_device_topology( + struct libxfs_dev *dev, + struct device_topology *dt, int force_overwrite) { - struct stat statbuf; + struct stat st; + + /* + * Nothing to do if this particular subvolume doesn't exist. + */ + if (!dev->name) + return; /* * If our target is a regular file, use platform_findsizes * to try to obtain the underlying filesystem's requirements * for direct IO; we'll set our sector size to that if possible. */ - if (xi->data.isfile || - (!stat(xi->data.name, &statbuf) && S_ISREG(statbuf.st_mode))) { - int fd; + if (dev->isfile || (!stat(dev->name, &st) && S_ISREG(st.st_mode))) { int flags = O_RDONLY; long long dummy; + int fd; /* with xi->disfile we may not have the file yet! */ - if (xi->data.isfile) + if (dev->isfile) flags |= O_CREAT; - fd = open(xi->data.name, flags, 0666); + fd = open(dev->name, flags, 0666); if (fd >= 0) { - platform_findsizes(xi->data.name, fd, &dummy, - &ft->lsectorsize); + platform_findsizes(dev->name, fd, &dummy, + &dt->logical_sector_size); close(fd); - ft->psectorsize = ft->lsectorsize; - } else - ft->psectorsize = ft->lsectorsize = BBSIZE; + } else { + dt->logical_sector_size = BBSIZE; + } } else { - blkid_get_topology(xi->data.name, &ft->dsunit, &ft->dswidth, - &ft->lsectorsize, &ft->psectorsize, + blkid_get_topology(dev->name, &dt->sunit, &dt->swidth, + &dt->logical_sector_size, + &dt->physical_sector_size, force_overwrite); } - if (xi->rt.name && !xi->rt.isfile) { - int sunit, lsectorsize, psectorsize; + ASSERT(dt->logical_sector_size); - blkid_get_topology(xi->rt.name, &sunit, &ft->rtswidth, - &lsectorsize, &psectorsize, force_overwrite); - } + /* + * Older kernels may not have physical/logical distinction. + */ + if (!dt->physical_sector_size) + dt->physical_sector_size = dt->logical_sector_size; +} + +void +get_topology( + struct libxfs_init *xi, + struct fs_topology *ft, + int force_overwrite) +{ + get_device_topology(&xi->data, &ft->data, force_overwrite); + get_device_topology(&xi->rt, &ft->rt, force_overwrite); } diff --git a/libxfs/topology.h b/libxfs/topology.h index 3a309a4da..ba0c8f669 100644 --- a/libxfs/topology.h +++ b/libxfs/topology.h @@ -10,12 +10,16 @@ /* * Device topology information. */ +struct device_topology { + int logical_sector_size; /* logical sector size */ + int physical_sector_size; /* physical sector size */ + int sunit; /* stripe unit */ + int swidth; /* stripe width */ +}; + struct fs_topology { - int dsunit; /* stripe unit - data subvolume */ - int dswidth; /* stripe width - data subvolume */ - int rtswidth; /* stripe width - rt subvolume */ - int lsectorsize; /* logical sector size &*/ - int psectorsize; /* physical sector size */ + struct device_topology data; + struct device_topology rt; }; void diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index fcbf54132..be65ccc1e 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -1986,31 +1986,24 @@ validate_sectorsize( * than that, then we can use logical, but warn about the * inefficiency. * - * Set the topology sectors if they were not probed to the - * minimum supported sector size. - */ - if (!ft->lsectorsize) - ft->lsectorsize = dft->sectorsize; - - /* - * 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; + if (ft->data.physical_sector_size > XFS_MAX_SECTORSIZE) { + ft->data.physical_sector_size = + ft->data.logical_sector_size; + } - cfg->sectorsize = ft->psectorsize; + cfg->sectorsize = ft->data.physical_sector_size; if (cfg->blocksize < cfg->sectorsize && - cfg->blocksize >= ft->lsectorsize) { + cfg->blocksize >= ft->data.logical_sector_size) { fprintf(stderr, _("specified blocksize %d is less than device physical sector size %d\n" "switching to logical sector size %d\n"), - cfg->blocksize, ft->psectorsize, - ft->lsectorsize); - cfg->sectorsize = ft->lsectorsize; + cfg->blocksize, ft->data.physical_sector_size, + ft->data.logical_sector_size); + cfg->sectorsize = ft->data.logical_sector_size; } } else cfg->sectorsize = cli->sectorsize; @@ -2031,9 +2024,9 @@ _("block size %d cannot be smaller than sector size %d\n"), usage(); } - if (cfg->sectorsize < ft->lsectorsize) { + if (cfg->sectorsize < ft->data.logical_sector_size) { fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"), - cfg->sectorsize, ft->lsectorsize); + cfg->sectorsize, ft->data.logical_sector_size); usage(); } } @@ -2455,7 +2448,7 @@ validate_rtextsize( if (!cfg->sb_feat.nortalign && !cli->xi->rt.isfile && !(!cli->rtsize && cli->xi->data.isfile)) - rswidth = ft->rtswidth; + rswidth = ft->rt.swidth; else rswidth = 0; @@ -2700,13 +2693,14 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( /* if no stripe config set, use the device default */ if (!dsunit) { /* Ignore nonsense from device report. */ - if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit), - BBTOB(ft->dswidth), 0, true)) { + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->data.sunit), + BBTOB(ft->data.swidth), 0, true)) { fprintf(stderr, _("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), - progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); - ft->dsunit = 0; - ft->dswidth = 0; + progname, + BBTOB(ft->data.sunit), BBTOB(ft->data.swidth)); + ft->data.sunit = 0; + ft->data.swidth = 0; } else if (cfg->dblocks < GIGABYTES(1, cfg->blocklog)) { /* * Don't use automatic stripe detection if the device @@ -2714,29 +2708,29 @@ _("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\ * on such a small system are not worth the risk that * we'll end up with an undersized log. */ - if (ft->dsunit || ft->dswidth) + if (ft->data.sunit || ft->data.swidth) fprintf(stderr, _("%s: small data volume, ignoring data volume stripe unit %d and stripe width %d\n"), - progname, ft->dsunit, - ft->dswidth); - ft->dsunit = 0; - ft->dswidth = 0; + progname, ft->data.sunit, + ft->data.swidth); + ft->data.sunit = 0; + ft->data.swidth = 0; } else { - dsunit = ft->dsunit; - dswidth = ft->dswidth; + dsunit = ft->data.sunit; + dswidth = ft->data.swidth; use_dev = true; } } else { /* check and warn if user-specified alignment is sub-optimal */ - if (ft->dsunit && ft->dsunit != dsunit) { + if (ft->data.sunit && ft->data.sunit != dsunit) { fprintf(stderr, _("%s: Specified data stripe unit %d is not the same as the volume stripe unit %d\n"), - progname, dsunit, ft->dsunit); + progname, dsunit, ft->data.sunit); } - if (ft->dswidth && ft->dswidth != dswidth) { + if (ft->data.swidth && ft->data.swidth != dswidth) { fprintf(stderr, _("%s: Specified data stripe width %d is not the same as the volume stripe width %d\n"), - progname, dswidth, ft->dswidth); + progname, dswidth, ft->data.swidth); } } diff --git a/repair/sb.c b/repair/sb.c index dedac53af..02c10d9a5 100644 --- a/repair/sb.c +++ b/repair/sb.c @@ -189,7 +189,7 @@ guess_default_geometry( * Use default block size (2^12) */ blocklog = 12; - multidisk = ft.dswidth | ft.dsunit; + multidisk = ft.data.swidth | ft.data.sunit; dblocks = x->data.size >> (blocklog - BBSHIFT); calc_default_ag_geometry(blocklog, dblocks, multidisk, agsize, agcount);
fs_topology is a mess that mixes up data and RT device reporting, and to make things worse reuses lsectorsize for the logical sector size while other parts of xfsprogs use it for the log sector size. Split out a device_topology structure that reports the topology for one device and embedded two of them into the fs_topology struture. Rename the sector size members to be more explicit, and move some of the sanity checking from mkfs into the topology helpers. Signed-off-by: Christoph Hellwig <hch@lst.de> --- libxfs/topology.c | 61 ++++++++++++++++++++++++++++---------------- libxfs/topology.h | 14 +++++++---- mkfs/xfs_mkfs.c | 64 +++++++++++++++++++++-------------------------- repair/sb.c | 2 +- 4 files changed, 78 insertions(+), 63 deletions(-)