lkml.org 
[lkml]   [2003]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/4] Optimize NFS open() calls by means of 'intents'...

On Fri, 23 May 2003, Trond Myklebust wrote:
>
> Minor cleanup of open() code. Put the original open flags, mode, etc. into
> an 'opendata' structure that can be passed as an intent to lookup.

I don't mind the concepts, but I _really_ dislike the implementation.

For one thing, if you're creating a structure to pass in the flags for
open, then you should take the time to make the code _more_ readable
rather than less. In particular, the notion of having a structure like
this:

struct opendata {
int flag;
int mode;
int acc_mode;
};

where each of "flag" and "acc_mode" are magic bitfields just fills me with
horror.

So why not make those internal modes that we translate the "flags" into be
a real bitmap? That should make the code a lot more readable.

Also, I don't really understand why you want to have "opendata" and
"intent" as different structures. That's _especially_ true now that the
only intent is the "open" intent, but even if there were other intents,
I'd rather have something like this

struct lookup_info {
enum type; /* open, validate, whatever.. */
union {
struct open_intent open;
..
} data;
}

and gace tge flags (create/exclusive etc) inside that lookup_intent
instead of having multiple different pointers and transferring data from
one to the other at different phases of the "open".

Also, in patch 3/4, you do

xxx_create(struct inode *dir, struct dentry *dentry, int mode, struct vfsintent *intent)

and that "mode" this I again find offensive: why is it not in the intent?
It automatically _would_ be, if you only had one structure and one
pointer, but you lost it when you did the "opendata->intent"
transformation.

So please don't have this artifical (and clearly broken) differentiation
between "intent" and "opendata". They should be one and the same thing:
"lookup_info". Because that is what they _are_. They are not intents. They
are literally extra information for the lookup.

Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:35    [W:0.037 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site