From mboxrd@z Thu Jan 1 00:00:00 1970 From: Louis Rilling Subject: Re: [Ocfs2-devel] [RFC] configfs: Pin configfs subsystems separately from new config_items. Date: Fri, 20 Jun 2008 14:46:44 +0200 Message-ID: <20080620124644.GQ30804@localhost> References: <1213742460-26331-1-git-send-email-joel.becker@oracle.com> <20080618123134.GC30804@localhost> <20080618161215.GA16780@ca-server1.us.oracle.com> <20080618165101.GI30804@localhost> <20080618200713.GE16780@ca-server1.us.oracle.com> <20080619111357.GM30804@localhost> <20080619220739.GC10888@mail.oracle.com> Reply-To: Louis.Rilling@kerlabs.com Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=_bohort-5650-1213965914-0001-2" Cc: linux-fsdevel@vger.kernel.org, cluster-devel@redhat.com, ocfs2-devel@oss.oracle.com To: Joel.Becker@oracle.com Return-path: Received: from bohort.kerlabs.com ([62.160.40.57]:58949 "EHLO bohort.kerlabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752955AbYFTMqr (ORCPT ); Fri, 20 Jun 2008 08:46:47 -0400 Content-Disposition: inline In-Reply-To: <20080619220739.GC10888@mail.oracle.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: This is a MIME-formatted message. If you see this text it means that your E-mail software does not support MIME-formatted messages. --=_bohort-5650-1213965914-0001-2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 19, 2008 at 03:07:39PM -0700, Joel Becker wrote: > On Thu, Jun 19, 2008 at 01:13:57PM +0200, Louis Rilling wrote: > > On Wed, Jun 18, 2008 at 01:07:13PM -0700, Joel Becker wrote: > > > > 4/ make_item()/make_group() pins the module of the new item/group i= f it differs > > > > from the current one, and at least until drop_item() (must check= in-tree > > > > subsystems, but I suspect that module dependency tracking alread= y does the > > > > job). > > >=20 > > > This is a silly rule, though. Why "pin if it is different" when > > > "always pin" is just as effective and much easier to understand? You > > > never have to ask "but was the item's module pinned?" when tracking a > > > problem. > >=20 > > Not so silly, if you consider that this relieves in-tree users from hav= ing to > > add try_module_get() in their code. Only special users (like me) who cr= eate > > items implemented by other modules, without explicitly depending on sym= bols of > > these modules or keeping references after ->drop_item(), have to deal w= ith > > such module pinning. >=20 > With my rule ("always pin"), single-module users don't have to > try_module_get() at all. Just like today. That's kind of my point. With my rules, neither single-module users, nor in-tree multi-module users = have to try_module_get(). And, as I try to convince you of, configfs does not ha= ve to try_module_get() on new items. What I suggest is a *simplification* of configfs, and does not require to c= hange existing subsystems. I'm convinced that this does not lower the safety of configfs, but instead this makes it clear that subsystems have to take care= of module pinning, as they always had to. >=20 > > And I think that we can also get rid of the last config_item_put() (or > > put it before client_drop_item()), because after client_drop_item() rmd= ir() does > > not need the item anymore, and client_drop_item() is supposed to call > > config_item_put() (in parent's drop_item() or directly). IOW, when ente= ring > > rmdir() configfs already holds the item's ref that was given by parent's > > ->make_item(), and rmdir() drops that ref in client_drop_item(). No nee= d to hold > > the extra one grabbed by configfs_get_config_item(). >=20 > We could, but it's a much cleaner read to hold a reference for > the duration of the function. And since we hold a module reference > anyway, I like simpler and clearer. But keeping this last config_item_put() prevents the simplification that I'm defending. And I think that the simplification is worth moving this config_item_put() before client_drop_item(). >=20 > > > In the end, we are holding a reference to the module while we > > > are accessing it. It's overkill for the common case (single module w= as > > > safe before when we pinned item->owner, and it is safe if we only > > > pin subsys->owner), but it provides the best proper safety if we allow > > > multi-module subsystems. > >=20 > > As I said above, the way it is done currently, pinning the new item's m= odule > > does not provide any safety in multi-module subsystems. >=20 > We provide safety for ourselves. We can't provide safety for > the subsystem - we don't know how it is put together. Once again, the > module reference is just configfs saying "I know that I have a reference > and that I'm safe to access this." No, we do not provide safety for ourselves: remember the scenario I describ= ed earlier in this thread [ Actually I'm able to show you a similar scenario w= ith a non-preemptible kernel on a single-processor system, because attach_group()= may block and schedule in a memory allocation. ]. This is the reason why I sugg= est to get rid of the last config_item_put() above, and simply remove this new item's module pinning which, again, does not provide any safety, even for configfs alone. Note that I agree with your principle of providing safety for configfs only, and let modules handle their specific module uses. I'm really keeping= this in mind, and also keeping in mind not to disturb subsystems with new rules.= It just happens that the rules that I'm proposing are already respected by in-= tree subsystems, and are even already respected by my own multi-modules use case. So, what do you think is really bad in my suggestion? Louis --=20 Dr Louis Rilling Kerlabs Skype: louis.rilling Batiment Germanium Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes http://www.kerlabs.com/ 35700 Rennes --=_bohort-5650-1213965914-0001-2 Content-Type: application/pgp-signature; name="signature.asc" Content-Transfer-Encoding: 7bit Content-Description: Digital signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIW6a0VKcRuvQ9Q1QRAiP8AKCYKm8HdA27lDs7EDXxQ2PQAZKcwQCcC4Mt n7vmeupiGS/wh78K+tDSwHE= =KkEX -----END PGP SIGNATURE----- --=_bohort-5650-1213965914-0001-2--