Lustre devel - Dec 2006 - [Bug 10796] dbench 16 oops with lustre exported over NFS

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


green@clusterfs.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #9144|review?(green@clusterfs.com)|review+
               Flag|                            |


(From update of attachment 9144)
>--- namei.c	24 Nov 2006 12:01:41 -0000	1.170.2.15.2.36
>+++ namei.c	14 Dec 2006 16:29:41 -0000
>@@ -363,31 +363,47 @@ static void ll_d_add(struct dentry *de, 
>+                if (dentry->d_flags & DCACHE_DISCONNECTED) {
>+                        BUG_ON(last_discon != NULL);
LASSERT(last_discon == NULL);

see my comments about patch for bug 11135 in that bug. (also it is generally
bad idea to supply
same patches in different bugs).

Aside from that BUG_ON(), patch looks good.
Ideally we need to find out why d_instantiate_unique fails when we use it,
though.

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 #9144 is|0                           |1
           obsolete|                            |


Created an attachment (id=9159)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9159&action=view)
more nfs fixes

updated patch fix panic in d_move, when unhashed dentry pased into vfs_rename.

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 #9159|                            |review?(green@clusterfs.com)
               Flag|                            |

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 #9159|review?(green@clusterfs.com)|review-
               Flag|                            |


(From update of attachment 9159)
You need to unhash newly added dentries, so that they could not be found and
are forced to be revalidated from servers the hard way. Leaving dentries in
cache with no lock would lead to problems where another client renames the
object, and then on original client you do stat on renamed name, and then on
original name and both come true, because original name has dentry hashed and
d_revalidate_it for this dentry will return true as we have locks on it
already.

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 #9159 is|0                           |1
           obsolete|                            |


Created an attachment (id=9185)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9185&action=view)
update patch

Don`t hash dentry in create hard/soft-link, or node. Don`t use lustre dcache
functions.

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 #9185 is|0                           |1
           obsolete|                            |


Created an attachment (id=9215)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9215&action=view)
more nfs fixes

Update nfs fixes patch to fix problem with run test sanity 99 at NFS.
NFS want to override files with permission 0444 but mds_open don`t allow this.
Also merged fix from cmd3 "always apply umask when doing open/create on
revalidate"

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 #9215 is|0                           |1
           obsolete|                            |


Created an attachment (id=9268)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9268&action=view)
more nfs fixes

Updated comments for ll_file_open and ll_find_aliases.
recheck stack usage for ll_find_alias via checkstack.pl it`s show 64 for old
and
new variant.

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?(green@clusterfs.com)|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?)

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 is|0                           |1
           obsolete|                            |


Created an attachment (id=9319)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9319&action=view)
reverted optimization and restore stack usage.