Re: svn commit: r302985 - head/sys/geom/label

[Maxim Sobolev]

Andrey, would you mind leaving your comments in the phabricator so that the
Mikhail (original contributor of the patch) could work on an improved
version? You might be correct about the abs(), I've overlooked that, but
the fs_fsize math is just copied over the previous code as far as I can
tell. So if it worked with the strict comparison, it should also work with
relaxed bound one.

https://reviews.freebsd.org/D6208

Thanks!

-Max

On Mon, Jul 18, 2016 at 1:18 PM, Andrey V. Elsukov 
wrote:

> On 18.07.16 22:37, Maxim Sobolev wrote:
> > Well, this looks to me exactly what I am talking about. With this change
> > we are only allowing underlying provider to be *slighly* bigger than the
> > UFS size. So as I said it's pretty harmless to do so, or at least I
>
> So, this isn't true.
> 1. You use abs() in the macro - it can be less or bigger.
> 2. fs_fsize is size of frag blocks, so use it as divider looks incorrect.
>
> --
> WBR, Andrey V. Elsukov
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r302985 - head/sys/geom/label

[Andrey V. Elsukov]

On 18.07.16 22:37, Maxim Sobolev wrote:
> Well, this looks to me exactly what I am talking about. With this change
> we are only allowing underlying provider to be *slighly* bigger than the
> UFS size. So as I said it's pretty harmless to do so, or at least I

So, this isn't true.
1. You use abs() in the macro - it can be less or bigger.
2. fs_fsize is size of frag blocks, so use it as divider looks incorrect.

-- 
WBR, Andrey V. Elsukov



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r302985 - head/sys/geom/label

[Maxim Sobolev]

Well, this looks to me exactly what I am talking about. With this change we
are only allowing underlying provider to be *slighly* bigger than the UFS
size. So as I said it's pretty harmless to do so, or at least I think it
is. In general I think this case is underlying some missing design feature
of GEOM framework, which basically gives provider to a first bidder that
likes the taste of it. What needs to be happening instead is to have some
rank in there, so if consumer A and consumer B both "like" the taste, but
provider A has bigger rank it would be given a preference. Then "whole
disk" partitioning schemes (mbr, gpt) could be given biggest rank, BSD
label and friends slightly smaller, encryption/compression yet smaller and
providers that just "decorate" things, like LABEL the lowest possible.

-Max

On Mon, Jul 18, 2016 at 12:13 PM, Andrey V. Elsukov 
wrote:

> On 18.07.16 17:24, Maxim Sobolev wrote:
> > Andrey, are you talking about this:
> >
> > ---
> > r156299 | pjd | 2006-03-04 11:41:54 -0800 (сб, 04 мар 2006) | 11 lines
> >
> > We need to check if file system size is equal to provider's size, because
> > sysinstall(8) still bogusly puts first partition at offset 0 instead of
> 16,
> > so glabel/ufs will find file system on slice instead of partition.
> >
> > Before sysinstall is fixed, we must keep this code, which means that we
> > wont't be able to detect UFS file systems created with 'newfs -s ...'.
> >
> > PS. bsdlabel(8) creates partitions properly.
> >
> > MFC after:  3 days
> > ---
> >
> > In which case this particular change has a better chance of working
> > since it's not removing this check but making it less strict. Therefore
> > it might attach to a wrong provider only if first UFS slice is the only
> > one slice on partition (or if the other partition is very small - less
> > than 256 blocks in size). In either of those cases I don't think it
> > makes much difference if we are attaching to a slice or a partition.
>
> No, I mean r235918, that was reverted after several complains.
> UFS label is a special label. It always had the same size that provider.
> Now it will attach to first provider that will be tasted. It can be
> gmirror, generic glabel, geli, gpart, mbr, whole disk.
>
> https://lists.freebsd.org/pipermail/freebsd-geom/2009-April/003473.html
>
> --
> WBR, Andrey V. Elsukov
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r302985 - head/sys/geom/label

[Andrey V. Elsukov]

On 18.07.16 17:24, Maxim Sobolev wrote:
> Andrey, are you talking about this:
> 
> ---
> r156299 | pjd | 2006-03-04 11:41:54 -0800 (сб, 04 мар 2006) | 11 lines
> 
> We need to check if file system size is equal to provider's size, because
> sysinstall(8) still bogusly puts first partition at offset 0 instead of 16,
> so glabel/ufs will find file system on slice instead of partition.
> 
> Before sysinstall is fixed, we must keep this code, which means that we
> wont't be able to detect UFS file systems created with 'newfs -s ...'.
> 
> PS. bsdlabel(8) creates partitions properly.
> 
> MFC after:  3 days
> ---
> 
> In which case this particular change has a better chance of working
> since it's not removing this check but making it less strict. Therefore
> it might attach to a wrong provider only if first UFS slice is the only
> one slice on partition (or if the other partition is very small - less
> than 256 blocks in size). In either of those cases I don't think it
> makes much difference if we are attaching to a slice or a partition.

No, I mean r235918, that was reverted after several complains.
UFS label is a special label. It always had the same size that provider.
Now it will attach to first provider that will be tasted. It can be
gmirror, generic glabel, geli, gpart, mbr, whole disk.

https://lists.freebsd.org/pipermail/freebsd-geom/2009-April/003473.html

-- 
WBR, Andrey V. Elsukov



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r302985 - head/sys/geom/label

[Maxim Sobolev]

I think people might be worried about existing deployed systems that might
stop booting if geom label obscures the whole disk making one or more
slices unavailable via geom_label mechanism.

-Max

On Mon, Jul 18, 2016 at 10:09 AM, Devin Teske  wrote:

>
> > On Jul 18, 2016, at 7:24 AM, Maxim Sobolev  wrote:
> >
> > Andrey, are you talking about this:
> >
> > ---
> > r156299 | pjd | 2006-03-04 11:41:54 -0800 (сб, 04 мар 2006) | 11 lines
> >
> > We need to check if file system size is equal to provider's size, because
> > sysinstall(8)
>
>
> ?? sysinstall ?? in head ??
>
> --
> Devin
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r302985 - head/sys/geom/label

[Devin Teske]

> On Jul 18, 2016, at 7:24 AM, Maxim Sobolev  wrote:
> 
> Andrey, are you talking about this:
> 
> ---
> r156299 | pjd | 2006-03-04 11:41:54 -0800 (сб, 04 мар 2006) | 11 lines
> 
> We need to check if file system size is equal to provider's size, because
> sysinstall(8)


?? sysinstall ?? in head ??

-- 
Devin

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r302985 - head/sys/geom/label

[Maxim Sobolev]

Andrey, are you talking about this:

---
r156299 | pjd | 2006-03-04 11:41:54 -0800 (сб, 04 мар 2006) | 11 lines

We need to check if file system size is equal to provider's size, because
sysinstall(8) still bogusly puts first partition at offset 0 instead of 16,
so glabel/ufs will find file system on slice instead of partition.

Before sysinstall is fixed, we must keep this code, which means that we
wont't be able to detect UFS file systems created with 'newfs -s ...'.

PS. bsdlabel(8) creates partitions properly.

MFC after:  3 days
---

In which case this particular change has a better chance of working since
it's not removing this check but making it less strict. Therefore it might
attach to a wrong provider only if first UFS slice is the only one slice on
partition (or if the other partition is very small - less than 256 blocks
in size). In either of those cases I don't think it makes much difference
if we are attaching to a slice or a partition.

-Maxim

On Sun, Jul 17, 2016 at 10:37 PM, Andrey V. Elsukov 
wrote:

> On 18.07.16 08:00, Maxim Sobolev wrote:
> > Author: sobomax
> > Date: Mon Jul 18 05:00:01 2016
> > New Revision: 302985
> > URL: https://svnweb.freebsd.org/changeset/base/302985
> >
> > Log:
> >   Relax checking if the privider size matches size recorded in the
> >   superblock, allowing provider to be bit bigger, i.e. have some
> >   extra padding after the FS image. That in some cases might be
> >   a side-effect of using CLOOP format which enforces certain block
> >   size and trying to compress image that is not exactly the number
> >   of those blocks in size. The UFS itself does not have any issues
> >   mounting such padded file systems, so it's what GEOM_LABEL should
> >   do.
>
> While you are thinking this is good fix, I expect that this change will
> break many installations. This is not first time when this check was
> changed.
>
> --
> WBR, Andrey V. Elsukov
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r302985 - head/sys/geom/label

[Andrey V. Elsukov]

On 18.07.16 08:00, Maxim Sobolev wrote:
> Author: sobomax
> Date: Mon Jul 18 05:00:01 2016
> New Revision: 302985
> URL: https://svnweb.freebsd.org/changeset/base/302985
> 
> Log:
>   Relax checking if the privider size matches size recorded in the
>   superblock, allowing provider to be bit bigger, i.e. have some
>   extra padding after the FS image. That in some cases might be
>   a side-effect of using CLOOP format which enforces certain block
>   size and trying to compress image that is not exactly the number
>   of those blocks in size. The UFS itself does not have any issues
>   mounting such padded file systems, so it's what GEOM_LABEL should
>   do.

While you are thinking this is good fix, I expect that this change will
break many installations. This is not first time when this check was
changed.

-- 
WBR, Andrey V. Elsukov



signature.asc
Description: OpenPGP digital signature

svn commit: r302985 - head/sys/geom/label

[Maxim Sobolev]

Author: sobomax
Date: Mon Jul 18 05:00:01 2016
New Revision: 302985
URL: https://svnweb.freebsd.org/changeset/base/302985

Log:
  Relax checking if the privider size matches size recorded in the
  superblock, allowing provider to be bit bigger, i.e. have some
  extra padding after the FS image. That in some cases might be
  a side-effect of using CLOOP format which enforces certain block
  size and trying to compress image that is not exactly the number
  of those blocks in size. The UFS itself does not have any issues
  mounting such padded file systems, so it's what GEOM_LABEL should
  do.
  
  Submitted by: @mizhka_gmail.com
  Differential Revision:https://reviews.freebsd.org/D6208

Modified:
  head/sys/geom/label/g_label_ufs.c

Modified: head/sys/geom/label/g_label_ufs.c
==
--- head/sys/geom/label/g_label_ufs.c   Mon Jul 18 04:36:18 2016
(r302984)
+++ head/sys/geom/label/g_label_ufs.c   Mon Jul 18 05:00:01 2016
(r302985)
@@ -45,6 +45,15 @@ __FBSDID("$FreeBSD$");
 #defineG_LABEL_UFS_VOLUME  0
 #defineG_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
+ */
+#defineG_LABEL_UFS_CMP(prov, fsys, size)   
   \
+   ( abs( ((fsys)->size) - ( (prov)->mediasize / (fsys)->fs_fsize ))  \
+   < G_LABEL_UFS_MAXDIFF )
+#defineG_LABEL_UFS_MAXDIFF 0x100
+
 static const int superblocks[] = SBLOCKSEARCH;
 
 static void
@@ -82,18 +91,35 @@ g_label_ufs_taste_common(struct g_consum
if (fs == NULL)
continue;
/*
-* Check for magic. We also need to check if file system size 
is 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.
+* 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).
 */
+   switch (fs->fs_magic){
+   case FS_UFS1_MAGIC:
+   case FS_UFS2_MAGIC:
+   G_LABEL_DEBUG(1, "%s %s params: %jd, %d, %d, %jd\n",
+   fs->fs_magic == FS_UFS1_MAGIC ? "UFS1" : "UFS2",
+   pp->name, pp->mediasize, fs->fs_fsize,
+   fs->fs_old_size, fs->fs_providersize);
+   break;
+   default:
+   break;
+   }
+
if (fs->fs_magic == FS_UFS1_MAGIC && fs->fs_fsize > 0 &&
-   ((pp->mediasize / fs->fs_fsize == fs->fs_old_size) ||
-   (pp->mediasize / fs->fs_fsize == fs->fs_providersize))) {
+   ( G_LABEL_UFS_CMP(pp, fs, fs_old_size)
+   || G_LABEL_UFS_CMP(pp, fs, fs_providersize))) {
/* Valid UFS1. */
} else if (fs->fs_magic == FS_UFS2_MAGIC && fs->fs_fsize > 0 &&
-   ((pp->mediasize / fs->fs_fsize == fs->fs_size) ||
-   (pp->mediasize / fs->fs_fsize == fs->fs_providersize))) {
+   ( G_LABEL_UFS_CMP(pp, fs, fs_size)
+   || G_LABEL_UFS_CMP(pp, fs, fs_providersize))) {
/* Valid UFS2. */
} else {
g_free(fs);
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"