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 #9341|review?([EMAIL PROTECTED])|review+
Flag| |
(From update of attachment 9341)
my comments are pretty minor this time.
I decided to correct some of your comments in hopes it would be easier to read
them later.
The only big change I still think we might need is that d_rehash();d_unhash();
change to init_list_head
of some sort, may be with configure check. Otherwise is good.
>@@ -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.
calls
>+ * filp_open call dentry_open after open_namei where check
>result
filp_open calls dentry_open after call to open_namei that checks for
permissions.
>+ * permision call. only nfsd_open call dentry_open directly
>without
nfsd_open calls...
>+ * check permision and because it this check is safe.
checking permissions and because of that this code below is safe.
>@@ -492,8 +501,7 @@ out_och_free:
> }
> up(&lli->lli_och_sem);
> }
>-
>- return rc;
>+ RETURN(rc);
> }
this change is wrong. Please drop it, we cannot reach it without going through
GOTO() and thta will print rc
in the log already.
>@@ -475,7 +477,13 @@ static int lookup_it_finish(struct ptlrp
> ll_d_add(*de, inode);
> spin_unlock(&dcache_lock);
> } else {
>- (*de)->d_inode = NULL;
>+ /* Lustre don`t want hash dentry if don`t have lock,
We do not want to hash the dentry is we don't have a lock.
>+ * but if this dentry used for d_move kernel got
But if this dentry is later used in d_move, we'd hit uninitialised list head
>+ * panic when tried access to dentry->d_hash and
>d_hash is NULL.
d_hash, so we just do this to
>+ * init d_hash field but leave dentry unhashed. (bug
>10796)
>+ */
>+ d_rehash(*de);
>+ d_drop(*de);
Now, I understand that we are trying to avoid having a configure check(s) as
different kernels might
have this different, but taking dcache lock twice is not exactly cheap either.
So it worth at least putting a comment here that just initialiseing list head
is possible here.
(in case we ever face perf problems due to this later)
Amd just doing a list head init is probably still much more desirable even at
the expense of one more configure check.
>+static int ll_new_node(struct inode *dir, struct qstr *name,
>+ char *tgt, int mode,
>+ int rdev, struct dentry *dchild)
> {
...
>+
>+ if (dchild) {
>+ err = ll_prep_inode(sbi->ll_osc_exp, &inode, request, 0,
>+ dchild->d_sb);
>+ if (err)
>+ GOTO(err_exit, err);
>+
>+ d_drop(dchild);
>+ d_instantiate(dchild, inode);
>+ EXIT;
>+ }
you need to move that EXIT here, after the bracket.
_______________________________________________
Lustre-devel mailing list
[email protected]
https://mail.clusterfs.com/mailman/listinfo/lustre-devel
