From owner-svn-src-head@FreeBSD.ORG Mon Jul 9 07:23:55 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C8920106564A; Mon, 9 Jul 2012 07:23:55 +0000 (UTC) (envelope-from etnapierala@googlemail.com) Received: from mail-ey0-f182.google.com (mail-ey0-f182.google.com [209.85.215.182]) by mx1.freebsd.org (Postfix) with ESMTP id C6DB88FC19; Mon, 9 Jul 2012 07:23:54 +0000 (UTC) Received: by eabm6 with SMTP id m6so4375689eab.13 for ; Mon, 09 Jul 2012 00:23:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20120113; h=sender:subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to:x-mailer; bh=pSoSM/RnlWUSHZRFb261v9rB5GbUVUc4z4DAmfag3Pk=; b=zEDgAMeZ9OIzchn5aNX3eAM0NzxYsTpCwMYMFZjozxDWOaNlIyqNpZnKd0cczrWjAT 4k3I8a489CkxtrIWEy1obtJZz1O//VXeLh8NUEHvYcDMUftVyl25ZadGc4N2AwRQTnJ1 +fTkYzq7mLWHfa3ivKVlrCLnpIEz9AZYpZqe0S9cRr6ZOvFuIT/nvEAxc5XkOCYstWi+ /iUC7CzVu5hfFc+tfRrrbR87A6RiZwUIvdSRRB2Lo0bAOO0Dy6sJJzS/G9iNnpom3twc wT7rflCCv/J2m6Rhsz2iVm09Q3r6exYnqwE2hdJmtNJP5GYSgKOF4eMch5itTXcNO4WQ lRaw== Received: by 10.14.188.4 with SMTP id z4mr9403275eem.228.1341818628197; Mon, 09 Jul 2012 00:23:48 -0700 (PDT) Received: from apn-77-113-49-165.dynamic.gprs.plus.pl (apn-77-113-49-165.dynamic.gprs.plus.pl. [77.113.49.165]) by mx.google.com with ESMTPS id e48sm91189803eea.12.2012.07.09.00.23.33 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 09 Jul 2012 00:23:47 -0700 (PDT) Sender: =?UTF-8?Q?Edward_Tomasz_Napiera=C5=82a?= Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset=iso-8859-2 From: =?iso-8859-2?Q?Edward_Tomasz_Napiera=B3a?= In-Reply-To: <20120708213846.GF1437@garage.freebsd.pl> Date: Mon, 9 Jul 2012 09:19:29 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: <739062A0-4339-46A3-AF8E-193BC57758BA@FreeBSD.org> References: <201207072013.q67KDfHN082943@svn.freebsd.org> <20120707215424.GE1437@garage.freebsd.pl> <280C8AEE-F7E8-4AAE-87BF-E59D0249B74F@FreeBSD.org> <20120708213846.GF1437@garage.freebsd.pl> To: Pawel Jakub Dawidek X-Mailer: Apple Mail (2.1278) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r238213 - head/sys/geom X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jul 2012 07:23:55 -0000 Wiadomo=B6=E6 napisana przez Pawel Jakub Dawidek w dniu 8 lip 2012, o = godz. 23:38: > On Sun, Jul 08, 2012 at 12:53:37AM +0200, Edward Tomasz Napiera=B3a = wrote: >> Wiadomo=B6=E6 napisana przez Pawel Jakub Dawidek w dniu 7 lip 2012, o = godz. 23:54: >>> You will also notice that one of those fields were left for more >>> universal method to handle various provider's property changes (ie. >>> provider's name, apart from its mediasize). The initial patch was = even >>> published a year ago: >>>=20 >>> = http://people.freebsd.org/~pjd/patches/geom_property_change.patch >>>=20 >>> Even if it was somehow totally not reusable it would at least give = you a >>> hint that mediasize is not the only thing that can change and if we = are >>> making that change it should be done right. >>=20 >> I was not aware of that patch. [...] >=20 > I'm afraid that's a lie. =46rom IRC logs: >=20 > rwatson: = http://people.freebsd.org/~pjd/patches/geom_property_change.patch > rwatson: Not tested. > pjd: shouldn't there also be a flag for geom to veto resizing? > pjd: for classes that can't handle their consumers changing = size? > [the discussion was pretty long] And when exactly was that? A year ago? >> [...] What I've considered was to use attributes >> instead, but that would complicate notifying consumers about resizing >> and would require some special-casing in the attribute code. >=20 > What attributes? The ones handled by BIO_GETATTR? They are about > something totally different. Yes, but they could be used to notify about mediasize change. Now, I'm not sure if I like your approach. You're trying to generalize from a single case. And even for that single case your approach would require a special case to retaste the provider after updating mediasize, and perhaps do the orphanisation stuff before. Also, why would we want this generalisation? Resize handling is = completely different from e.g. rename handling; why should we have a single method to do both? It's not that geom structures are like mbufs or vnodes, = where every byte counts. >>>> + G_VALID_PROVIDER(pp); >>>=20 >>> Is this your protection from a provider going away? >>=20 >> Can you suggest a way to do it in a safe way that doesn't involve >> rewriting most of GEOM? >=20 > I can only suggest not to rewrite GEOM because you didn't take the = time > to understand it. Let me quote a comment from the top of geom_event.c: /* * XXX: How do we in general know that objects referenced in events * have not been destroyed before we get around to handle the event ? */ So, can you suggest a way to do it in a safe way that doesn't involve rewriting most of GEOM? >>> Why is this safe to call the orphan method directly and not use >>> g_orphan_provider()? I don't know if using g_orphan_provider() is = safe >>> to use here either, but I'm under impression that you assume no = orphan >>> method will ever drop the topology lock? We have tools to assert = that, >>> no need to introduce such weak assumptions. >>=20 >> It's not that using g_orphan_provider() would be safer here - it = simply >> wouldn't work. The way it works is by adding providers to a queue >> (g_doorstep). _Providers_, and we need to orphan individual = consumers. >> So, this would involve rewriting the orphanisation mechanism. Also, >> most of the classes were fixed by mav@ to handle this correctly, = IIRC. >=20 > By introducing such hacks you make the code unpredictable. The way > g_orphan_provider() works is more than just calling geom's orphan > method. Also, until now, when orphan method was called it meant that > provider is going away, which is not true anymore. I'd like to believe > that you carefully analysed what you changed here is safe, but based = on > your understanding of GEOM, I doubt that. Look, I really appreciate you're looking at this just six months after explicitly refusing to talk to me about the design, but it would be = great if it was a _technical_ discussion. Now, the only reason for the orphaning during resizing is backward compatibility with classes that don't know anything about the resize() method, to make sure the provider doesn't get shrunk without them = knowing.=20 Your patch didn't do that, and perhaps we could just get rid of it. I think the current approach is safer, though. --=20 If you cut off my head, what would I say? Me and my head, or me and my = body?