[lustre-devel] Remaining work needed for moving Lustre out of staging

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Dec 06, 2016 at 12:50:08PM -0500, Oleg Drokin wrote:
> 
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
> 
> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
> >> 
> >> On Dec 3, 2016, at 3:55 AM, gregkh at linuxfoundation.org wrote:
> >> 
> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
> >>>> Al,
> >>>> Greg recently raised the issue of what still needs to be done to
> >>>> move the Lustre code out of staging/ and into the fs/ tree. 
> >>>> 
> >>>> James has been doing a great job of cleaning up various checkpatch
> >>>> issues and keeping the code updated with the latest fixes, but we
> >>>> were wondering what you were aware of that needed to be cleaned
> >>>> up in Lustre?
> >>> 
> >>> Is the whole "mixing kernel structures in userspace structures" all
> >>> resolved now?  For some reason I thought that you had kernel locks being
> >>> passed to userspace and then back into the kernel, but it's been a long
> >>> time since I last looked...
> >> 
> >> While we certainly had our share of mixing user/kernelspace structures,
> >> I don't think we ever passed anything with locks around back and forth.
> >> 
> >> I just did a brief check and I don't see anything glaring on this particular front.
> >> 
> >>> If you feel you are ready for a "real" review, I'll be glad to go over
> >>> the code before the vfs people look at it, just let me know.  No need to
> >>> bother them if you still have basic things wrong that I can find?
> >> 
> >> I think this would be beneficial at this stage.
> > 
> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
> > all of them up first?
> > 
> 
> There are 16 errors,
> of them:
> 8 are false positives in  drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
> 
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> +                       for (end_dirent = ent; ent;
> +                            end_dirent = ent, ent = lu_dirent_next(ent));

That's odd, I think the perl script just got confused.

> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
> 
> I'll have patches shortly for these.
> 
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a multiline thing anyway.

Change to use a "sane" logging function and then all of those will go
away.  You all should do that anyway, the macro mess you have now is
looney.

> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
> a bunch of comment style problems
> a few "function definition argument ? should also have an identifier name" that
> seems to be a new one too, I don't remember seeing this before.
> 
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false positives too.
> Do you really want all of these fixed too somehow?

The real ones, yes.  Why wouldn't you?

And work on that logging mess if at all possible as well :)

thanks,

greg k-h