From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Marshall Subject: Re: Remaining work needed for moving Lustre out of staging Date: Tue, 6 Dec 2016 13:07:40 -0500 Message-ID: References: <1E1B324A-F545-417D-9C35-550FA58665C7@intel.com> <20161203085550.GA5375@kroah.com> <0F617FFB-4BD0-4477-890B-2CDC56436DCC@intel.com> <20161206161524.GA5535@kroah.com> <7AF464A9-1D0F-48E9-B83C-66F983268473@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3180315077682416625==" Cc: linux-fsdevel , Greg KH , "lustre-devel-aLEFhgZF4x6X6Mz3xDxJMA@public.gmane.org List" , James Simmons To: Oleg Drokin Return-path: In-Reply-To: <7AF464A9-1D0F-48E9-B83C-66F983268473-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: lustre-devel-bounces-aLEFhgZF4x6X6Mz3xDxJMA@public.gmane.org Sender: "lustre-devel" List-Id: linux-fsdevel.vger.kernel.org --===============3180315077682416625== Content-Type: multipart/alternative; boundary=001a1142bf1abcf13605430147c0 --001a1142bf1abcf13605430147c0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable > long error messages that we cannot split into a multiline thing anyway. I split numerous Orangefs ones at substitution characters, which didn't break grepability. No one complained, and sure made stuff look nicer to me. You might be able to do that some places? -Mike On Tue, Dec 6, 2016 at 12:50 PM, 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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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 lo= ng > >>> 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 ove= r > >>> 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=E2= =80=A6 > >> > >> I think this would be beneficial at this stage. > > > > I see loads of checkpatch.pl warnings and a few errors, how about fixin= g > > 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 =3D ent; ent; > + end_dirent =3D ent, ent =3D lu_dirent_next(e= nt)); > > 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. > 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 =E2=80=A6 should also have an identif= ier 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? > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" = in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > --001a1142bf1abcf13605430147c0 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
> long error messages that we canno= t split into a multiline thing anyway.

I split numerous Orange= fs ones at substitution characters, which
didn't break grepabi= lity. No one complained, and sure made
stuff look nicer to me. You= might be able to do that some places?

-Mike

On Tue, Dec 6, 2016 at 12= :50 PM, Oleg Drokin <oleg.drokin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org wrote:
>>
>>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrot= e:
>>>> Al,
>>>> Greg recently raised the issue of what still needs to be d= one to
>>>> move the Lustre code out of staging/ and into the fs/ tree= .
>>>>
>>>> James has been doing a great job of cleaning up various ch= eckpatch
>>>> issues and keeping the code updated with the latest fixes,= but we
>>>> were wondering what you were aware of that needed to be cl= eaned
>>>> up in Lustre?
>>>
>>> Is the whole "mixing kernel structures in userspace struc= tures" all
>>> resolved now?=C2=A0 For some reason I thought that you had ker= nel 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 struct= ures,
>> I don't think we ever passed anything with locks around back a= nd forth.
>>
>> I just did a brief check and I don't see anything glaring on t= his 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.= =C2=A0 No need to
>>> bother them if you still have basic things wrong that I can fi= nd=E2=80=A6
>>
>> I think this would be beneficial at this stage.
>
> I see loads of checkpatch.pl warnings and a few errors, how about fixin= g
> all of them up first?
>

There are 16 errors,
of them:
8 are false positives in=C2=A0 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/lustr= e_idl.h
1 I think false positive in drivers/staging/lustre/lustre/include/lust= re/lustre_idl.h (PLDLMRES)
2 false positives in drivers/staging/lustre/lustre/include/lustre/lust= re_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: +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0for (end_dirent =3D ent; ent;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 end_dirent =3D ent, ent =3D lu_dirent_next(ent));<= br>
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/sock= lnd/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 multili= ne thing anyway.
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 =E2=80=A6 should also have an iden= tifier 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?

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdeve= l" in
the body of a message to major= domo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at=C2=A0 http://vger.kernel.org/m= ajordomo-info.html

--001a1142bf1abcf13605430147c0-- --===============3180315077682416625== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ lustre-devel mailing list lustre-devel-aLEFhgZF4x6X6Mz3xDxJMA@public.gmane.org http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org --===============3180315077682416625==--