Hello! Please, please, please, let's try to finally get some of these patches installed before discussing matters to death. Of course, discussion is very important -- and many thanks to Olaf et al. for doing all these reviews! -- but I'm totally losing track of all these emails and huge discussion threads and proposals and misunderstandings and clarifications of them and further possibilities.
On Mon, Aug 03, 2009 at 09:19:17PM +0300, Sergiu Ivanov wrote:
> On Sat, Jul 18, 2009 at 08:08:20AM +0200, olafbuddenha...@gmx.net wrote:
> > On Sun, Jul 12, 2009 at 10:50:07PM +0300, Sergiu Ivanov wrote:
> > > On Sat, Jul 11, 2009 at 03:56:02AM +0200, olafbuddenha...@gmx.net wrote:
> > > > On Wed, Jul 08, 2009 at 10:30:41PM +0300, Sergiu Ivanov wrote:
> > So there is no other way to associate the two lists? This is ugly
> > indeed. In this case, I think it would be better not to use the iterator
> > at all -- what you did here looks really hackish, and it breaks the
> > iterator paradigm anyways...
>
> Yeah, true, it breaks the paradigm. However, I actually borrowed this
> piece of code from node_init_root (node.c), so this is the style used
> in unionfs. Should I forget about consistency in this case, what do
> you think?
Go for consistency, and add something like ``TODO: this should be
rewritten like this: ..., because ...''. Olaf, you're of course also
very much encouraged to add such comments in code that you have reviewed.
Then it is written down, right next to the code, and doesn't have to be
investiagted again and again later on.
> > > > > @@ -290,7 +322,10 @@ netfs_attempt_sync (struct iouser *cred, struct
> > > > > node *np,
> > > > > error_t
> > > > > netfs_attempt_syncfs (struct iouser *cred, int wait)
> > > > > {
> > > > > - return 0;
> > > > > + /* The complete list of ports to the merged filesystems is
> > > > > + maintained in the root node of unionfs, so if we sync it, we
> > > > > sync
> > > > > + every single merged directory. */
> > > > > + return netfs_attempt_sync (cred, netfs_root_node, wait);
> > > >
> > > > I'm don't think this is really the right approach. Why not forward the
> > > > syncfs to all unioned filesystems?
Why would that be the wrong approach?
> Ah, so you mean forwarding syncfs to all unioned *directories*?
> Sorry, I thought your were talking about doing fsys_syncfs on all
> unioned *filesystems* :-)
>
> In this case, I'd tell you that the current implementation does
> exactly what you are talking about: sends file_sync to all writeable
> directories maintained by unionfs :-) You see, the list of *ports* to
> the directories is stored in the root node of unionfs (only), so doing
> netfs_attempt_sync on netfs_root_node is a pretty natural choice,
> IMHO. The ULFS list (maintained in ulfs.[ch]) is actually only a list
> of paths and attributes.
Yes, I think that this is exactly the right thing to do: sync everything
that is reachable from the root node.
> > > OTOH, I'm not sure whether syncing the whole filesystem is that very
> > > good, because in this way we lose the specificity: it's very probable
> > > that such syncing will employ a larger number of directories than just
> > > those merged by unionfs.
> >
> > So? Syncing more than necessary is never a problem... (Except for
> > performance perhaps.)
> >
> > Syncing is safety measure -- it can be too little, but it can never be
> > too much :-)
But why do that in the first place? Or am I misunderstanding something?
So. Sergiu, if you need specific review of some parts of this patch (or
any other patches), then please say so, otherwise please get it
installed. I assume that you can confirm in some way (testing, staring
at the code, ...) that it does the correct thing. And should there be
any breakage, and we discover it later on, we'll repair it later on. We
can't address or even fix all potential design or implementation
odditites of the unionfs code in this single discussion thread.
I hope that these words of mine aren't seen as an affront against the
lots of time that you people invest in discussing patches, writing
emails, etc. -- which is absolutely not my intention! -- but, yeah, let's
get something DONE! :-)
Regards,
Thomas
signature.asc
Description: Digital signature
