Please don't reply to lustre-devel. Instead, comment in Bugzilla by using the
following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10796
What |Removed |Added
----------------------------------------------------------------------------
Attachment #9268|review?([EMAIL PROTECTED])|review-
Flag| |
(From update of attachment 9268)
>diff -u -p -r1.127.2.32.2.65 file.c
>--- lustre/llite/file.c 15 Dec 2006 21:17:53 -0000 1.127.2.32.2.65
>+++ lustre/llite/file.c 4 Jan 2007 11:53:16 -0000
>@@ -383,10 +383,15 @@ int ll_file_open(struct inode *inode, st
>- if (oit.it_flags & O_CREAT)
>+ /* kernel only call f_op->open in dentry_open.
>+ * filp_open call dentry_open after open_namei where check
>result
>+ * permision call. only nfsd_open call dentry_open directly
>without
>+ * check permision and because it this check is safe.
>+ */
>+ if (file->f_flags & O_WRONLY)
I think you need to drop the if completely.
I see nothing that would prevent user from doing open(file, O_RDWR|O_CREAT,
000), for example.
(or even O_RDONLY)
> oit.it_flags |= MDS_OPEN_OWNEROVERRIDE;
>
> /* We do not want O_EXCL here, presumably we opened the file
>@@ -492,8 +501,7 @@ out_och_free:
> }
> up(&lli->lli_och_sem);
> }
>-
>- return rc;
>+ RETURN(rc);
This part is wrong. We cannot reach here without going through GOTO() macro,
and that will print the rc.
> struct dentry *ll_find_alias(struct inode *inode, struct dentry *de)
> {
> struct list_head *tmp;
>+ struct dentry *parent = de->d_parent;
>+ struct dentry *dentry;
>+ int len = de->d_name.len;
>+ const char *name = de->d_name.name;
>+ unsigned int hash = de->d_name.hash;
>+ struct dentry *last_discon = NULL;
>+ struct qstr *qstr;
Your stack measurements were made on x86_64 which has much more free registers
for local variables.
on i386 the picture is different. And i386 is the arch where we can see 4k
stacks too.
I think no need to do local optimisations like this. Smart compilers can do
this job too.
>-#else
>-struct dentry *ll_find_alias(struct inode *inode, struct dentry *de)
>-{
>- struct dentry *dentry;
>-
>- dentry = d_add_unique(de, inode);
>- if (dentry) {
>- lock_dentry(dentry);
>- dentry->d_flags &= ~DCACHE_LUSTRE_INVALID;
>- unlock_dentry(dentry);
>- }
>-
>- return dentry?dentry:de;
>-}
>-#endif
I think we better leave this code here with the comment above that it cannot be
used yet.
May be one day we would be able to remove that LBUG from d_instantiate_unique,
that poisons us,
or just change our way of doing things.
>@@ -470,7 +478,8 @@ static int lookup_it_finish(struct ptlrp
> ll_d_add(*de, inode);
> spin_unlock(&dcache_lock);
> } else {
>- (*de)->d_inode = NULL;
>+ d_rehash(*de);
>+ d_drop(*de);
And this misses some comment to explain why we need it.
(btw, is not it easier to just do some sort of INIT_LIST_HEAD() here?)
_______________________________________________
Lustre-devel mailing list
[email protected]
https://mail.clusterfs.com/mailman/listinfo/lustre-devel
