Please don't reply to lustre-devel. Instead, comment in Bugzilla by using the
following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11135
What |Removed |Added
----------------------------------------------------------------------------
Attachment #9118|review?([EMAIL PROTECTED]|review+
Flag|m) |
(From update of attachment 9118)
>+static int ll_new_node(struct inode *dir, struct qstr *name,
>+ struct qstr *tgt, int mode,
>+ int rdev, struct dentry *dchild)
In the end I'm not sure if passing a qstr is an improvement or not. It does
make the API marginally better, at the expense of extra stack space. New way
is (qstr = pointer + len + hash) on the stack and qstr pointer as parameter
vs. (pointer + int) as parameter, so new way is 8 or 12 bytes more stack usage.
We don't use the qstr directly anywhere internal to ll_new_inode() so there
isn't a win in that regard. Lustre is very stack constrained, and it is
usually best to avoid stack usage where possible, so I would just pass the
tgt_name and tgt_len directly as we did before.
> {
> struct ptlrpc_request *request = NULL;
> struct inode *inode = NULL;
> struct ll_sb_info *sbi = ll_i2sbi(dir);
> struct mdc_op_data op_data;
> int err;
>+
>+ ENTRY;
>+
>+ ll_prepare_mdc_op_data(&op_data, dir, NULL, name->name,
>+ name->len, 0);
>+ err = mdc_create(sbi->ll_mdc_exp, &op_data, tgt->name, tgt->len,
>+ mode, current->fsuid, current->fsgid,
>+ current->cap_effective, rdev, &request);
>+ if (err)
>+ goto err_exit;
In Lustre code it is preferred to use GOTO(err_exit, err) so that the debug
logs record which exit path was taken.
>+ ll_update_times(request, 0, dir);
>+
>+ if (dchild) {
>+ err = ll_prep_inode(sbi->ll_osc_exp, &inode, request, 0,
>+ dchild->d_sb);
>+ if (err)
>+ goto err_exit;
>+
>+ d_instantiate(dchild, inode);
>+ }
Here you would put an EXIT, we know that err = 0 here.
>+err_exit:
>+ ptlrpc_req_finished(request);
>+
>+ RETURN(err);
And here you would just have "return err", since we know from the GOTO() macro
what the error is and it is considered an "exit".
Otherwise it looks good and nice clean code. Is there a test that we can use
for this in sanity.sh (e.g. checking on client if there is an NFS mount,
creating a symlink to that, doing ls).
_______________________________________________
Lustre-devel mailing list
[email protected]
https://mail.clusterfs.com/mailman/listinfo/lustre-devel
