cvs commit: src/sys/amd64/conf GENERIC src/sys/i386/conf GENERIC src/sys/ia64/conf GENERIC src/sys/pc98/conf GENERIC src/sys/powerpc/conf GENERIC src/sys/sparc64/conf GENERIC src/sys/sun4v/conf GENERIC

[Pawel Jakub Dawidek]

On Tue, Feb 27, 2007 at 10:20:52AM +0100, Dag-Erling Sm?rgrav wrote:
> Brooks Davis <brooks at FreeBSD.org> writes:
> > While I agree there are serious problems with glabel and software RAID1
> > configurations, I don't think that warrants continuing to hide it from
> > the rest of us.  We should probably add more warnings to the appropriate
> > manpages and fix the RAID implementations.
> 
> The problem isn't just with the RAID implementations.  It goes deeper
> than that.
> 
> First, when geom_label sees multiple identical labels, it ignores all
> but the first one.  The old implementation (geom_vol_ffs) had comments
> in the source code pointing this out:
> 
>                 /* XXX We need to check for namespace conflicts. */
>                 /* XXX How do you handle a mirror set? */
>                 /* XXX We don't validate the volume name. */
>                 g_topology_lock();
>                 /* Alright, we have a label and a volume name, reconfig. */
>                 g_slice_config(gp, 0, G_SLICE_CONFIG_SET, (off_t) 0,
>                     pp->mediasize, pp->sectorsize, "vol/%s",
>                     fs->fs_volname);
>                 g_free(fs);
>                 g_topology_unlock();
> 
> The new implementation has the same bug / feature, but does not
> document it:
> 
>         snprintf(name, sizeof(name), "%s/%s", dir, label);
>         LIST_FOREACH(gp, &mp->geom, geom) {
>                 pp2 = LIST_FIRST(&gp->provider);
>                 if (pp2 == NULL)
>                         continue;
>                 if (strcmp(pp2->name, name) == 0) {
>                         G_LABEL_DEBUG(1, "Label %s(%s) already exists (%s).",
>                             label, name, pp->name);
>                         if (req != NULL) {
>                                 gctl_error(req, "Provider %s already exists.",
>                                     name);
>                         }
>                         return (NULL);
>                 }
>         }
> 
> In addition, the issue is never logged; the debugging message is
> normally disabled, and the error message is ignored when req is NULL
> (req is always NULL when tasting existing labels; it is non-NULL only
> when creating a new label using 'glabel create')
> 
> This is exacerbated by the fact that ataraid does not hide the
> underlying devices when an array is configured, and they are usually
> tasted before the array, so you are pretty much guaranteed that
> geom_label attaches to the wrong provider.
> 
> (this same fact also leads to spurious and confusing error messages
> from other GEOM classes, such as "corrupt or invalid GPT detected"
> when tasting the first component of a RAID 0 array that contains a
> GPT)

Dag-Erling, you're proposing removing it from GENERIC, because ataraid
doesn't work nicely in the current world order. From what I looked some
time ago ataraid isn't using GEOM to access components. AFAIR at some
point ataraid was hidding components. Gmirror/graid3 for example opens
all components for write+exclusive which prevents such mistakes.

Anyway, if we decide to remove glabel from GENERIC, I'd at least like to
make the consensus clear - ataraid should be changed to fit better in
what we currently have.

-- 
Pawel Jakub Dawidek                       http://www.wheel.pl
pjd at FreeBSD.org                           http://www.FreeBSD.org
FreeBSD committer                         Am I Evil? Yes, I Am!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20070227/7bb681bf/attachment.pgp