Lustre devel - Dec 2006 - [Bug 10930] Implement star support for Lustre EAs

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10930

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #8467 is|0                           |1
           obsolete|                            |


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

updated patch for lustre per adilger''s suggestion.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10930

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #8466 is|0                           |1
           obsolete|                            |
Attachment #8955 is|0                           |1
           obsolete|                            |


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

updated patch for star per adilger''s suggestion

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10930

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #9195|review?(adilger@clusterfs.co|review+
               Flag|m)                          |


(From update of attachment 9195)
>+int ll_dir_getstripe(struct inode *inode, struct lov_mds_md **lmmp, 
>+                     int *lmm_size, struct ptlrpc_request **request) 
>+{
>+        rc = mdc_getattr(sbi->ll_mdc_exp, &fid,
>+                        OBD_MD_FLEASIZE|OBD_MD_FLDIREA,
>+                        lmmsize, &req);
Please align arguments with ''('' on previous line.
>+        body = lustre_msg_buf(req->rq_repmsg, REPLY_REC_OFF,
>+                        sizeof(*body));
Align arguments with ''(''
>+        if (!(body->valid & (OBD_MD_FLEASIZE | OBD_MD_FLDIREA)) ||
>+                        lmmsize == 0) {
Also align with ''if ('' on previous line.
>+        lmm = lustre_msg_buf(req->rq_repmsg, REPLY_REC_OFF + 1,
>+                        lmmsize);
Same.
>@@ -488,44 +559,29 @@ static int ll_dir_ioctl(struct inode *in
>+                        rc = ll_lov_getstripe_ea_info(inode, filename,
&lmm, &lmmsize, &request);
Wrap at 80 columns.
>+                if(rc < 0){
>+                        if(rc == -ENODATA && (cmd ==
IOC_MDC_GETFILEINFO ||
>+                           cmd == LL_IOC_MDC_GETINFO))
Align with ''(cmd'' on previous line.
>+int ll_lov_getstripe_ea_info(struct inode *inode, const char *filename, 
>+                             struct lov_mds_md **lmmp, int *lmm_size, 
>+                             struct ptlrpc_request **request)
>+{
>+        rc = mdc_getattr_name(sbi->ll_mdc_exp, &fid,
>+                        filename, strlen(filename) + 1,
>+                        OBD_MD_FLEASIZE | OBD_MD_FLDIREA,
>+                        lmmsize, &req);
Please align arguments with ''('' after mdc_getattr_name.
>@@ -149,6 +149,27 @@ int ll_setxattr(struct dentry *dentry, c
>+        if (strncmp(name, XATTR_TRUSTED_PREFIX, 8) == 0 &&
>+                        strcmp(name + 8, "lov") == 0){
Align with ''if (''
>+                        rc = ll_lov_setstripe_ea_info(inode, &f, flags,
lump, sizeof(struct lov_user_md));
Wrap at 80 columns.
>+                } 
>+                else if(S_ISDIR(inode->i_mode)){
>+                        rc = ll_dir_setstripe(inode, lump);
Put ''} else if (...'' on same line.
>@@ -284,12 +305,53 @@ ssize_t ll_getxattr(struct dentry *dentr
>+         if (strncmp(name, XATTR_TRUSTED_PREFIX, 8) == 0 &&
>+             strcmp(name + 8, "lov") == 0){
Space between ''== 0) {''.
>+                 if ( S_ISREG(inode->i_mode)) {
>+                         const char *filename = dentry->d_name.name;
>+                         struct dentry *parent = dentry->d_parent;
>+                         rc = ll_lov_getstripe_ea_info(parent->d_inode,
filename, &lmm, &lmmsize, &request);
Please don''t use temporary variables here, Lustre is very short of
stack space.
 Also needs to wrap at 80 columns.


At this point does the client already have lli->lli_smd?  That would mean we
don''t need to do an RPC to get the stripe data at all, because it was
done
during the inode lookup or stat and the client already has it.	Can you please
do a simple test with a file in the root directory and check how many MDS RPCs
it does for this inode?
>+                 }
>+                 else if (S_ISDIR(inode->i_mode)) {
>+                         rc = ll_dir_getstripe(inode, &lmm,
&lmmsize, &request);
>+                 } 
>+                 else { /* symlink ? */
>+                         return ENODATA;
Put ''} else ...'' on same line.	Should this be -ENODATA?
> ssize_t ll_listxattr(struct dentry *dentry, char *buffer, size_t size)
> {
>+        int rc = 0, rc1=0;
Space around '' = ''.  Please use "rc2" or
"err", as that is more common in other
parts of the code.
>@@ -297,6 +359,44 @@ ssize_t ll_listxattr(struct dentry *dent
>+        rc = ll_getxattr_common(inode, NULL, buffer, size,
OBD_MD_FLXATTRLS);
>+
>+        if(!capable(CAP_SYS_ADMIN)){
>+                        if(lsm == NULL)
Space between ''if ('' everywhere.
>+                        size_t prefix_len = sizeof(XATTR_TRUSTED_PREFIX)-1;
>+                        size_t name_len   = sizeof("lov") -1;
>+                        size_t total_len = prefix_len + name_len + 1;
Please use "const size_t" and maybe compiler will optimize this away.

Does this need to be done for even root users?	When I run "getfattr -d
/mnt/lustre/foo" it doesn''t show trusted.lov even as root.  I have
to
specifically ask for it with "-n trusted.lov".

Most of the changes are not defects, just needed to match the CodingGuidelines
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
on the wiki: https://mail.clusterfs.com/wikis/lustre/CodingGuidelines

Can you please write some simple regression tests for sanity.sh (not using
star) to verify that the NEW functionality introduced here works correctly. 
That means getxattr -d (if installed) should return the trusted.lov EA for
regular files as root and non-root users, and mknod+setfattr should set the
striping correctly.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10930



if add one "nolustre" flag, seems "lustre" flag is no longer
useful: if working
with lustre file system, striping EA is get/set by default, if
"nolustre" so
just disable setting striping EA. I think either "lustre" or
"nolustre" need be
added to the command line option, but not both.

no output of "getfattr -d pathname", i find it is the problem of
getfattr, in
its source code "trusted." EA will never be printed though listxattr()
has
gotten the name.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10930

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #9195 is|0                           |1
           obsolete|                            |


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

fixed bad code style
always return 0 for regular file when ll_setxattr

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10930

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #9196 is|0                           |1
           obsolete|                            |


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

fixed bad code styles
keep setmodes() no changes
fixed defects per adilger''s suggestions

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10930

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #9232|review?(adilger@clusterfs.co|review+
               Flag|m)                          |


(From update of attachment 9232)
Huangwei, this can land on b1_5 when there are some regression tests in
sanity.sh that verify getfattr/setfattr of trusted.lov as root and non-root
users for regular files and directories.  Use mcreate to create files via
mknod.

If star is installed on the test system , and "star --help" (or
whatever)
reports lustre support (e.g. "star --help | grep lustre") then you
should do
additional tests that creates several files with different striping, backs them
up, and restores them to a different directories (one each with the default
options and with the --preserve_osts option) and then comparing the striping
information returned from "lfs getstripe -v" on the original and new
directory
(possibly with suitable manipulation from grep/sed/awk).