git: af433832f752 - main - geom_label: Remove an old sysinstall(8) workaround

[Jessica Clarke]

On 5 Jul 2021, at 19:56, Cy Schubert <Cy.Schubert at cschubert.com> wrote:
> 
> In message <E77F3B91-8B43-4936-AFFB-B96C8580467A at freebsd.org>, Jessica 
> Clarke w
> rites:
>> On 5 Jul 2021, at 19:36, Cy Schubert <Cy.Schubert at cschubert.com> wrote:
>>> 
>>> In message <202107051517.165FHsfD012512 at gitrepo.freebsd.org>, Jessica 
>>> Clarke wr
>>> ites:
>>>> The branch main has been updated by jrtc27:
>>>> 
>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=af433832f7520840c22edd1fe1266
>> c1a
>>>> 5cb781ad
>>>> 
>>>> commit af433832f7520840c22edd1fe1266c1a5cb781ad
>>>> Author:     Jessica Clarke <jrtc27 at FreeBSD.org>
>>>> AuthorDate: 2021-07-05 15:15:32 +0000
>>>> Commit:     Jessica Clarke <jrtc27 at FreeBSD.org>
>>>> CommitDate: 2021-07-05 15:15:32 +0000
>>>> 
>>>>   geom_label: Remove an old sysinstall(8) workaround
>>>> 
>>>>   We removed sysinstall(8) back in 2011, so this workaround should be lon
>> g
>>>>   since unnecessary. This workaround can end up breaking cases that are
>>>>   hit in the real world, such as dd'ing a small pre-built disk image to a
>>>>   large partition that you intend to grow on first boot and uses a UFS
>>>>   disk label for / in its /etc/fstab (as the only reliable thing a raw UF
>> S
>>>>   image can reference).
>>>> 
>>>>   Reviewed by:    imp, mckusick
>>>>   Differential Revision:  https://reviews.freebsd.org/D30825
>>>> ---
>>>> sys/geom/label/g_label_ufs.c | 35 +++++------------------------------
>>>> 1 file changed, 5 insertions(+), 30 deletions(-)
>>>> 
>>>> diff --git a/sys/geom/label/g_label_ufs.c b/sys/geom/label/g_label_ufs.c
>>>> index ababbaa4b43a..70d59488d7b6 100644
>>>> --- a/sys/geom/label/g_label_ufs.c
>>>> +++ b/sys/geom/label/g_label_ufs.c
>>>> @@ -49,19 +49,8 @@ __FBSDID("$FreeBSD$");
>>>> #define	G_LABEL_UFS_ID		1
>>>> 
>>>> /*
>>>> - * G_LABEL_UFS_CMP returns true if difference between provider mediasize
>>>> - * and filesystem size is less than G_LABEL_UFS_MAXDIFF sectors
>>>> - */
>>>> -#define	G_LABEL_UFS_CMP(prov, fsys, size) 			
>>>> 	   \
>>>> -	( abs( ((fsys)->size) - ( (prov)->mediasize / (fsys)->fs_fsize ))  \
>>>> -				< G_LABEL_UFS_MAXDIFF )
>>>> -#define	G_LABEL_UFS_MAXDIFF	0x100
>>>> -
>>>> -/*
>>>> - * Try to find a superblock on the provider. If successful, then
>>>> - * check that the size in the superblock corresponds to the size
>>>> - * of the underlying provider. Finally, look for a volume label
>>>> - * and create an appropriate provider based on that.
>>>> + * Try to find a superblock on the provider. If successful, look for a vo
>> lum
>>>> e
>>>> + * label and create an appropriate provider based on that.
>>>> */
>>>> static void
>>>> g_label_ufs_taste_common(struct g_consumer *cp, char *label, size_t size, 
>> in
>>>> t what)
>>>> @@ -81,24 +70,10 @@ g_label_ufs_taste_common(struct g_consumer *cp, char *
>> lab
>>>> el, size_t size, int wh
>>>> 		return;
>>>> 	}
>>>> 
>>>> -	/*
>>>> -	 * Check for magic. We also need to check if file system size
>>>> -	 * is almost equal to providers size, because sysinstall(8)
>>>> -	 * used to bogusly put first partition at offset 0
>>>> -	 * instead of 16, and glabel/ufs would find file system on slice
>>>> -	 * instead of partition.
>>>> -	 *
>>>> -	 * In addition, media size can be a bit bigger than file system
>>>> -	 * size. For instance, mkuzip can append bytes to align data
>>>> -	 * to large sector size (it improves compression rates).
>>>> -	 */
>>>> -	if (fs->fs_magic == FS_UFS1_MAGIC && fs->fs_fsize > 0 &&
>>>> -	    ( G_LABEL_UFS_CMP(pp, fs, fs_old_size)
>>>> -		|| G_LABEL_UFS_CMP(pp, fs, fs_providersize))) {
>>>> +	/* Check for magic. */
>>>> +	if (fs->fs_magic == FS_UFS1_MAGIC && fs->fs_fsize > 0) {
>>>> 		/* Valid UFS1. */
>>>> -	} else if (fs->fs_magic == FS_UFS2_MAGIC && fs->fs_fsize > 0 &&
>>>> -	    ( G_LABEL_UFS_CMP(pp, fs, fs_size)
>>>> -		|| G_LABEL_UFS_CMP(pp, fs, fs_providersize))) {
>>>> +	} else if (fs->fs_magic == FS_UFS2_MAGIC && fs->fs_fsize > 0) {
>>>> 		/* Valid UFS2. */
>>>> 	} else {
>>>> 		goto out;
>>>> 
>>> 
>>> On one of my machines which uses UFS filesystems (sandbox) also fails:
>>> 
>>> bob# ls /dev/ufs
>>> s10-amd64	s13-amd64	s13-amd64e	test
>>> s11-amd64	s13-amd64a	s13-amd64f	testa
>>> s12-amd64	s13-amd64d	s13-i386
>>> bob# mount /alt/s11-amd64/root
>>> bob# ls /dev/ufs
>>> s10-amd64	s12-amd64	test
>>> s11-amd64	s13-i386	testa
>>> bob# 
>>> 
>>> After the mount of /alt/s11-amd64/root, s10-amd64 the s13-* filesystems 
>>> become unavailble  They share the same fdisk slice, using different 
>>> bsdlabel partitions within the slice.
>>> 
>>> My laptop zfs pool is on a partition in the same slice as its UFS 
>>> filesystems. Looks like once a filesystem on a slice has been mounted, the 
>>> other partitions within the slice are no longer available following this 
>>> commit.
>> 
>> Hm, what does bsdlabel on the disk say? My guess is your disk is set up
>> in the bogus manner mentioned in that comment. Is it an old machine
>> installed by sysinstall(8)?
> 
> A little more investigation shows that on the two affected machines, UFS 
> partitions are at offset 0 of their respective slices. For legacy 
> environments this will require some kind of workaround or boot off a rescue 
> disk (which I do have) to dump/recreate/restore the partitions at an offset 
> that is not zero. The problem is quite logical.
> 
> Doing the work to recreate the partitions doesn't bother me but other 
> people may trip across this. Your call.

How about something like D31068?

Jess