RE: Staging: lustre: Make lustre_profile_list static

[Simmons, James A.]

>Variable lustre_profile_list is only used inside obd_config.c,
>better make it static
>
>Signed-off-by: Iban Rodriguez 

Acked-by: James Simmons 

>---
> drivers/staging/lustre/lustre/obdclass/obd_config.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/obd_config.c 
b/drivers/staging/lustre/lustre/obdclass/obd_config.c
index 5395e994deab..8d484633b83a 100644
--- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
+++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
@@ -606,7 +606,7 @@ static int class_del_conn(struct obd_device *obd, struct 
lustre_cfg *lcfg)
return rc;
 }
 
-LIST_HEAD(lustre_profile_list);
+static LIST_HEAD(lustre_profile_list);
 
 struct lustre_profile *class_get_profile(const char *prof)
 {
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [RFC PATCH 0/3] staging: lustre: detypedef

[Simmons, James A.]

>Question about removing lustre typedefs.
>
>Various bits of lustre code use a mix of struct foo and foo_t.
>
>When would be an appropriate time to submit patches similar to
>below that individually remove various typedefs from lustre code?
>
>These are pretty trivial to produce and verify so there's no
>particular hurry to do them now but applying them will require
>resync points for active and actually useful developers.

Actually could you hold off for the LNet core and LND drivers these
changes. I have plans to push a few more LNet patches soon. I have
been just waiting for everyone to figure out how to deal with the 
latest changes to the infinband layer first. So the plan is to push
FMR support for the ko2iblnd driver. Also we have additional work
too handle setting the size of the DMA pools for o2iblnd but that
patch touches some of the core LNet code as well. Once those are
landed we can look at removing most of the typedefs. When its
time for the typedef to be cleaned up lets do just the structs first.
There are a few typedefs like lnet_nid_t I like to keep or if it has to
be changed turn it into a struct then. Things like lnet_nid_t act like
a cookie handle.

Now the best place to do this cleanup right now is LNet selftest. No
new code is planned for landing. We have lots of typedefs to remove
and I was planning to do that cleanup but if you want to do it just CC
me, jsimm...@infradead.org, so I can test the changes.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 2/6] staging: lustre: libcfs: move memory_pressure functions to libcfs_prim.h

[Simmons, James A.]

>On Thu, Mar 31, 2016 at 10:18:36AM -0400, James Simmons wrote:
>> Long ago libcfs_prim.h was used for userland code which is why
>> memory_pressure_*() handling is in both libcfs_prim.h and
>> linux-mem.h headers. So lets just move the memory_pressure_*()
>> to libcfs_prim.h.
>> 
>> Signed-off-by: James Simmons 
>> ntel-bug-id: https://jira.hpdd.intel.com/browse/LU-6245
>
>"ntel-bug-id:"?  :)

Thanks for fixing that.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging/lustre/libcfs: Copy correct amount in libcfs_ioctl_getdata

[Simmons, James A.]

>Commit b8ff756bc351 ("staging: lustre: libcfs: merge code from
>libcfs_ioctl into libcfs_ioctl_getdata") introduced a problem
>copying just a single pointer worth of data from userspace
>instead of whole libcfs_ioctl_hdr structure.
>Adjust the copying amount.
>
>Signed-off-by: Oleg Drokin 

Acked- by: James Simmons 

>---
> drivers/staging/lustre/lnet/libcfs/linux/linux-module.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/lustre/lnet/libcfs/linux/linux-module.c 
>b/drivers/staging/lustre/lnet/libcfs/linux/linux-module.c
>index b86e937..d89f71e 100644
>--- a/drivers/staging/lustre/lnet/libcfs/linux/linux-module.c
>+++ b/drivers/staging/lustre/lnet/libcfs/linux/linux-module.c
>@@ -128,7 +128,7 @@ int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr **hdr_pp,
>   struct libcfs_ioctl_hdr hdr;
>   int err = 0;
>
>-  if (copy_from_user(, uhdr, sizeof(uhdr)))
>+  if (copy_from_user(, uhdr, sizeof(hdr)))
>   return -EFAULT;
> 
>   if (hdr.ioc_version != LIBCFS_IOCTL_VERSION &&
>-- 
>2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] Revert "Staging: lustre: o2iblnd: Use sizeof type *pointer instead of sizeof type."

[Simmons, James A.]

>> > > so the right code should be:
>> > > 
>> > > sizeof(**net->ibn_tx_ps);
>> > > and the same for sizeof(**net->ibn_fmr_ps)
>> > That's a mess, isn't there some other way to fix this up to be more
>> > "obvious"?
>> This must have been encountered in the past. How was it handle in those
>> other cases?
>
>I fail to see why it's a mess.  It's just **
>and someone making a mistake.

I have no trouble with **.  If we revert it someone else will come along and
do the same mistake so I think we are stuck with the change to **.

>Removing the "typedef struct" uses from lustre
>would probably make a lot of this clearer though.

That along with a few hundred more patches heading Greg's way :-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] Revert "Staging: lustre: o2iblnd: Use sizeof type *pointer instead of sizeof type."

[Simmons, James A.]

>On Wed, Mar 23, 2016 at 05:39:36AM +, Dilger, Andreas wrote:
>> On 2016/03/22, 19:49, "lustre-devel on behalf of Greg Kroah-Hartman"
>> > gre...@linuxfoundation.org> wrote:
>> 
>> >On Tue, Mar 22, 2016 at 06:21:04PM -0400, James Simmons wrote:
>> >> Latest testing fails when using ko2iblnd. It was tracked down
>> >> to commit 4671a026616df26000f7d8ad2f2ea4b6de79263c.
>> >> 
>> >> This reverts commit 4671a026616df26000f7d8ad2f2ea4b6de79263c.
>> >> ---
>> >>  .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c|4 ++--
>> >>  1 files changed, 2 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >>b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> index 89f9390..0d32e65 100644
>> >> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> @@ -1968,7 +1968,7 @@ static int kiblnd_net_init_pools(kib_net_t *net,
>> >>__u32 *cpts, int ncpts)
>> >>*/
>> >>  
>> >>   net->ibn_fmr_ps = cfs_percpt_alloc(lnet_cpt_table(),
>> >> -sizeof(*net->ibn_fmr_ps));
>> >> +sizeof(kib_fmr_poolset_t));
>> >
>> >Ok, why is this revert needed?  Please give me a big huge comment about
>> >why this is not the same size of the variable being assigned to it,
>> >otherwise someone else is going to come along and make the exact same
>> >change again.
>> >
>> >>   if (!net->ibn_fmr_ps) {
>> >>   CERROR("Failed to allocate FMR pool array\n");
>> >>   rc = -ENOMEM;
>> >> @@ -1992,7 +1992,7 @@ static int kiblnd_net_init_pools(kib_net_t *net,
>> >>__u32 *cpts, int ncpts)
>> >>  
>> >>   create_tx_pool:
>> >>   net->ibn_tx_ps = cfs_percpt_alloc(lnet_cpt_table(),
>> >> -   sizeof(*net->ibn_tx_ps));
>> >> +   sizeof(kib_tx_poolset_t));
>> >
>> >Same here, why is this code wrong?
>> 
>> Looks like the declarations are:
>> 
>> kib_tx_poolset_t **ibn_tx_ps;/* tx pool-set */
>>  kib_fmr_poolset_t   **ibn_fmr_ps;   /* fmr pool-set */
>> 
>> 
>> 
>> so the right code should be:
>> 
>> sizeof(**net->ibn_tx_ps);
>> 
>> 
>> and the same for sizeof(**net->ibn_fmr_ps)
>
>That's a mess, isn't there some other way to fix this up to be more
>"obvious"?

This must have been encountered in the past. How was it handle in those
other cases?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: lustre: really make lustre dependent on LNet

[Simmons, James A.]

>A patch intended to add a dependency on LNET for lustre didn't
>actually do that and instead allowed configurations that contain
>lustre with lnet but without IPv4 support that subsequently
>fail to link:
>
>warning: (LUSTRE_FS) selects LNET which has unmet direct dependencies (STAGING 
>&& INET && m && MODULES)
>ERROR: "kernel_sendmsg" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "sock_create_lite" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "sock_release" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "release_sock" [drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] 
>undefined!
>ERROR: "kernel_sendmsg" 
>[drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] undefined!
>ERROR: "tcp_sendpage" [drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] 
>undefined!
>
>This adds the one-line change that was evidently missing from the
>commit, doing what was intended there to have a correct set of dependencies.
>
>Signed-off-by: Arnd Bergmann 
>Fixes: b08bb6bb5af5 ("staging: lustre: make lustre dependent on LNet")

Thanks for fixing that. I was surprised that select didn't manage the 
configuration automatically.

Acked-by: James Simmons 

>---
> drivers/staging/lustre/lustre/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/lustre/lustre/Kconfig 
>b/drivers/staging/lustre/lustre/Kconfig
>index a09b51ce8265..8ac7cd4d6fdb 100644
>--- a/drivers/staging/lustre/lustre/Kconfig
>+++ b/drivers/staging/lustre/lustre/Kconfig
>@@ -1,7 +1,7 @@
> config LUSTRE_FS
>   tristate "Lustre file system client support"
>   depends on m && !MIPS && !XTENSA && !SUPERH
>-  select LNET
>+  depends on LNET
>   select CRYPTO
>   select CRYPTO_CRC32
>   select CRYPTO_CRC32_PCLMUL if X86
>-- 
>2.7.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 00/10] Last batch of fixes for LNet

[Simmons, James A.]

>On Fri, Mar 04, 2016 at 09:09:40PM -0500, James Simmons wrote:
>> This batch merges the remaining LNet patches from the OpenSFS
>> branch for the upstream client. Once merged the LNet code
>> will be up to date with the latest production code. Only style
>> issues are remaining. Still future patches being developed
>> for LNet will be landed to the upstream client as soon as they
>> are ready after extensive testing.
>
>Please fix up the build issue, and figure out what went wrong with the
>__user patch that you sent and resend this series after reworking them.

I had a discussion with Oleg about the __user patch. It  appears that is
a bug in the production branch so that patch can be dropped.  As for the
build issues this has been a know issue for a awhile but nobody has
gotten around to fixing all the 32 bit issues. I guess it is time to fix that 
up.
I will send out new patches later after I'm doing testing them.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 00/27] Third batch of LNet fixes

[Simmons, James A.]

>On Wed, Mar 02, 2016 at 05:01:43PM -0500, James Simmons wrote:
>> This patch set merges all the fixes for the klnd drivers, socklnd and
>> o2iblnd, to what is currently used in production environments. Several
>> more fixes for the LNet core are also included with this patch set.
>
>I've applied the first 20, I never got patch 21 in the series, so please
>rebase and resend the remaining ones, properly numbered :)
>
>thanks,

Okay. I will send the remaining patches as a new patch series.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] staging: lustre: Support different ko2iblnd configs between systems

[Simmons, James A.]

>On Wed, Mar 02, 2016 at 05:02:04PM -0500, James Simmons wrote:
>> From: Jeremy Filizetti 
>> 
>> This patch adds suppoort for ko2iblnd to have different values for
>> peer_credits and map_on_demand between systems.
>
>Your subject has no number for this patch, is it really patch 21 in the
>series?

Yes it is really patch 21. The patch needed to be updated at the last
minute before I sent it out. Sorry about that mistake.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH v2 0/6] staging: lustre: update modinfo data

[Simmons, James A.]

>On Fri, Feb 26, 2016 at 06:11:07AM +, Drokin, Oleg wrote:
>> 
>> On Feb 26, 2016, at 1:03 AM, Greg Kroah-Hartman wrote:
>> 
>> > On Thu, Feb 25, 2016 at 08:07:06PM -0500, James Simmons wrote:
>> >> The module information for Lustre is stale or in some cases
>> >> completely missing. This collection of patches brings the
>> >> modinfo up to date as well as filling in any missing information.
>> >> This patch set has been redone to rebase it on Oleg's latest
>> >> patch set to avoid collisons in merging.
>> >> 
>> >> Andreas Dilger (4):
>> >>  staging: lustre: add missing MODULE_AUTHOR for LNet selftest module
>> >>  staging: lustre: update the MODULE_DESCRIPTION for all lustre modules
>> >>  staging: lustre: make module_init/exit naming consistent
>> >>  staging: lustre: update comment for lnet_lib_init/exit
>> >> 
>> >> James Simmons (2):
>> >>  staging: lustre: move module info to end of libcfs module.c file
>> >>  staging: lustre: update the MODULE_VERSION for all lustre modules
>> > 
>> > What changed to need a v2 of this series?
>> > 
>> > Please ALWAYS say what the difference is, don't expect us to "just
>> > know".
>> > 
>> > Please send a v3 of this, describing the changes, in the correct format,
>> > in each patch.  You know better than this…
>> 
>> I think it says above that the rebase was done on top of my patchset
>> to resolve the conflict that arose?
>
>Where?  Ugh, yeah, kind of, but where is the big v2: marking or some
>such thing like is required?  It's very easy to miss this (as I did.)
>Please make it easy for maintainers, not hard on them...

Sorry, in the future I will add a ChangeLog section for newer versions of 
patches.
The good news is the patches from Oleg's that conflicted landed already which
means the this patch set is ready to land.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 5/7] staging:lustre: simplify libcfs_psdev_[open|release]

[Simmons, James A.]

>> diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c 
>> b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> index 33f6036..64f0fbf 100644
>> --- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> +++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> @@ -98,30 +98,22 @@ int libcfs_ioctl_popdata(void *arg, void *data, int size)
>>  static int
>>  libcfs_psdev_open(struct inode *inode, struct file *file)
>>  {
>> -intrc = 0;
>> -
>>  if (!inode)
>>  return -EINVAL;
>> -if (libcfs_psdev_ops.p_open != NULL)
>> -rc = libcfs_psdev_ops.p_open(0, NULL);
>> -else
>> -return -EPERM;
>> -return rc;
>> +
>> +try_module_get(THIS_MODULE);
>
>Note, code like this is racy and incorrect and never needed, please fix
>this up properly (hint, set the module in the file operations.)
>
>Again, if you ever see code with that line, it is incorrect.

So simple

static struct file_operations libcfs_fops = {
.module  = THIS_MODULE,
.unlocked_ioctl = libcfs_psdev_ioctl,
};

With the open and release deleted should do the trick then.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse

[Simmons, James A.]

>> On Wed, Dec 09, 2015 at 10:38:13PM +0530, Niranjan Dighe wrote:
>>> The third argument to function kportal_memhog_alloc is expected to
>>> be gfp_t whereas the actual argument was unsigned int. Fix this by
>>> explicitly typecasting to gfp_t
>>>
>>> Signed-off-by: Niranjan Dighe 
>>> ---
>>>  drivers/staging/lustre/lustre/libcfs/module.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
>>> b/drivers/staging/lustre/lustre/libcfs/module.c
>>> index 96d9d46..9c79f6e 100644
>>> --- a/drivers/staging/lustre/lustre/libcfs/module.c
>>> +++ b/drivers/staging/lustre/lustre/libcfs/module.c
>>> @@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file 
>>> *pfile, unsigned long cmd,
>>>   /* XXX The ioc_flags is not GFP flags now, need to be 
>>> fixed */
>>>   err = kportal_memhog_alloc(pfile->private_data,
>>>  data->ioc_count,
>>> -data->ioc_flags);
>>> + (__force gfp_t)data->ioc_flags);
>>
>> No, please fix the type to be correct properly, like the comment says
>> needs to be done.
>>
>> thanks,
>>
>> greg k-h
>
>Hello Greg,
>
>I could see that the ioc_flags member of the struct libcfs_ioctl_data
>is used as gfp_t only in the
>case of the ioctl IOC_LIBCFS_MEMHOG. I can think of following ways to
>correct it -

IOC_LIBCFS_MEMHOG will be going away. Since this keeps coming up I will prepare 
some patches.
Especially now that out tools no longer uses these obsolete ioctls.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse

[Simmons, James A.]

>>>2. Is it OK to hardcode the appropriate gfp_t flags for the
>>>IOC_LIBCFS_MEMHOG, as the userspace
>>>seems to be taking the decision about the page allocation
>>>zone/strategy, is this what is intended?
>>
>> The memhog functionality is used to introduce memory pressure on a client
>> or server during operation to test error handling as well as memory
>> allocation deadlocks (e.g. GFP_KERNEL used where GFP_NOFS should be used).
>> There are other ways to do this in the kernel today, so all of the memhog
>> code could just be deleted I think.
>>
>> This looks like kportal_memhog_alloc(), kportal_memhog_free(),
>> IOC_LIBCFS_MEMHOG, and struct libcfs_device_userstate could be removed.
>>
>>
>> Cheers, Andreas
>>
>Thanks Andreas, I will send out a separate patch with the cleanup as
>you suggested.

I missed this email. I just sent the cleanup patches a bit ago.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 02/29] staging/lustre/lnet: Get rid of IOC_LIBCFS_DEBUG_PEER hack

[Simmons, James A.]

>From: Oleg Drokin 
>
>IOC_LIBCFS_DEBUG_PEER was added back in the stone ages to print debug
>statistics on a peer when peer timeout happens.
>Redo it properly as a separate LNet API call,
>also get rid of "ioctl" forwarding into the underlying LNDs,
>since no current LNDs implement this function anymore.
..
>diff --git a/drivers/staging/lustre/include/linux/lnet/api.h 
>b/drivers/staging/lustre/include/linux/lnet/api.h
>index 75285fd..fa5fad3 100644
>--- a/drivers/staging/lustre/include/linux/lnet/api.h
>+++ b/drivers/staging/lustre/include/linux/lnet/api.h
>@@ -197,6 +197,7 @@ int LNetGet(lnet_nid_t   
> int LNetSetLazyPortal(int portal);
> int LNetClearLazyPortal(int portal);
> int LNetCtl(unsigned int cmd, void *arg);
>+void LNetDebugPeer(lnet_process_id_t id);

We don't need this one line camel case wrapper.

> 
> /** @} lnet_misc */
>
 ...
>@@ -1436,6 +1406,12 @@ LNetCtl(unsigned int cmd, void *arg)
> }
> EXPORT_SYMBOL(LNetCtl);
 >
>+void LNetDebugPeer(lnet_process_id_t id)
>+{
>+  lnet_debug_peer(id.nid);
>+}
>+EXPORT_SYMBOL(LNetDebugPeer);

Lets just use lnet_debug_peer directly.

>+
> /**
>  * Retrieve the lnet_process_id_t ID of LNet interface at \a index. Note that
>  * all interfaces share a same PID, as requested by LNetNIInit().

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 02/29] staging/lustre/lnet: Get rid of IOC_LIBCFS_DEBUG_PEER hack

[Simmons, James A.]

>>> @@ -1436,6 +1406,12 @@ LNetCtl(unsigned int cmd, void *arg)
>>> }
>>> EXPORT_SYMBOL(LNetCtl);
>>> 
>>> +void LNetDebugPeer(lnet_process_id_t id)
>>> +{
>>> +   lnet_debug_peer(id.nid);
>>> +}
>>> +EXPORT_SYMBOL(LNetDebugPeer);
>> 
>> Lets just use lnet_debug_peer directly.
>
>It's not exported and this is not how we export functionality out of the LNet.
>
>The way I did it is the way LNet api is exposed. Why break the custom all of a 
>sudden and
>replace one hack with another?

Because camel case is frowned on. Do we have to expose camel case functions 
only? 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] staging: lustre: Remove unused memhog functionality

[Simmons, James A.]

>On 2015/12/23, 14:40, "Simmons, James A." <simmon...@ornl.gov> wrote:
>
>>>From: Niranjan Dighe <ndi...@visteon.com>
>>>
>>>Remove IOC_LIBCFS_MEMHOG ioctl functionality as it is no longer needed
>>>thereby
>>>making functions like - kportal_memhog_alloc(), kportal_memhog_free()
>>>and type -
>>>struct libcfs_device_userstate unused.
>>
>>Actually so much more can be cleaned up. This work is also being done at
>>
>>http://review.whamcloud.com/#/c/17492.
>>
>>I will update that patch and post it here as well.
>
>James, part of that patch is for the userspace tools, which isn't
>applicable here.  Also, it probably makes sense to delete the panic injection 
>as a
>separate patch, so I think this patch is good as-is for removing memhog.

After looking at the code I agree. At first I thought it would be a simple 
cleanup expansion
of this patch but I see a lot more cleanups coming after this. This work is the 
first step to
removing the cfs_psdev_* abstraction. I will submit the cleanups soon. For now 
this patch
can be merged.

Acked-by: James Simmons <jsimm...@infradead.org>

>
>
>Signed-off-by: Niranjan Dighe <ndi...@visteon.com>
>---
> .../lustre/include/linux/libcfs/libcfs_private.h   |5 -
> .../lustre/lustre/libcfs/linux/linux-module.c  |   14 +-
> drivers/staging/lustre/lustre/libcfs/module.c  |  139
>
> 3 files changed, 2 insertions(+), 156 deletions(-)
>
>diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>index d6273e1..e044d6f 100644
>--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>@@ -391,11 +391,6 @@ int cfs_percpt_atomic_summary(atomic_t **refs);
>  * Support for temporary event tracing with minimal Heisenberg effect.
>  * 
>*/
> 
>-struct libcfs_device_userstate {
>-  intldu_memhog_pages;
>-  struct page   *ldu_memhog_root_page;
>-};
>-
> #define MKSTR(ptr) ((ptr)) ? (ptr) : ""
> 
> static inline int cfs_size_round4(int val)
>diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>index 70a99cf0..eccfe8bd 100644
>--- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>+++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>@@ -98,14 +98,12 @@ int libcfs_ioctl_popdata(void *arg, void *data, int
>size)
> static int
> libcfs_psdev_open(struct inode *inode, struct file *file)
> {
>-  struct libcfs_device_userstate **pdu = NULL;
>   intrc = 0;
> 
>   if (!inode)
>   return -EINVAL;
>-  pdu = (struct libcfs_device_userstate **)>private_data;
>   if (libcfs_psdev_ops.p_open != NULL)
>-  rc = libcfs_psdev_ops.p_open(0, (void *)pdu);
>+  rc = libcfs_psdev_ops.p_open(0, NULL);
>   else
>   return -EPERM;
>   return rc;
>@@ -115,14 +113,12 @@ libcfs_psdev_open(struct inode *inode, struct file
>*file)
> static int
> libcfs_psdev_release(struct inode *inode, struct file *file)
> {
>-  struct libcfs_device_userstate *pdu;
>   intrc = 0;
> 
>   if (!inode)
>   return -EINVAL;
>-  pdu = file->private_data;
>   if (libcfs_psdev_ops.p_close != NULL)
>-  rc = libcfs_psdev_ops.p_close(0, (void *)pdu);
>+  rc = libcfs_psdev_ops.p_close(0, NULL);
>   else
>   rc = -EPERM;
>   return rc;
>@@ -152,14 +148,8 @@ static long libcfs_ioctl(struct file *file,
>   return -EPERM;
>   panic("debugctl-invoked panic");
>   return 0;
>-  case IOC_LIBCFS_MEMHOG:
>-  if (!capable(CFS_CAP_SYS_ADMIN))
>-  return -EPERM;
>-  /* go thought */
>   }
> 
>-  pfile.off = 0;
>-  pfile.private_data = file->private_data;
>   if (libcfs_psdev_ops.p_ioctl != NULL)
>   rc = libcfs_psdev_ops.p_ioctl(, cmd, (void *)arg);
>   else
>diff --git a/drivers/staging/lustre/lustre/libcfs/module.c
>b/drivers/staging/lustre/lustre/libcfs/module.c
>index 329d78c..0067e53 100644
>--- a/drivers/staging/lustre/lustre/libcfs/module.c
>+++ b/drivers/staging/lustre/lustre/libcfs/module.c
>@@ -68,142 +68,16 @@ MODULE_LICENSE("GPL");
> 
> static struct dentry *lnet_debugfs_root;
> 
>-static void kportal_memhog_free(struct libcfs_device_usersta

RE: [lustre-devel] [PATCH] staging: lustre: Remove unused memhog functionality

[Simmons, James A.]

>From: Niranjan Dighe 
>
>Remove IOC_LIBCFS_MEMHOG ioctl functionality as it is no longer needed thereby
>making functions like - kportal_memhog_alloc(), kportal_memhog_free() and type 
>-
>struct libcfs_device_userstate unused.

Actually so much more can be cleaned up. This work is also being done at 

http://review.whamcloud.com/#/c/17492.

I will update that patch and post it here as well.


Signed-off-by: Niranjan Dighe 
---
 .../lustre/include/linux/libcfs/libcfs_private.h   |5 -
 .../lustre/lustre/libcfs/linux/linux-module.c  |   14 +-
 drivers/staging/lustre/lustre/libcfs/module.c  |  139 
 3 files changed, 2 insertions(+), 156 deletions(-)

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h 
b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
index d6273e1..e044d6f 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
@@ -391,11 +391,6 @@ int cfs_percpt_atomic_summary(atomic_t **refs);
  * Support for temporary event tracing with minimal Heisenberg effect.
  *  */
 
-struct libcfs_device_userstate {
-   intldu_memhog_pages;
-   struct page   *ldu_memhog_root_page;
-};
-
 #define MKSTR(ptr) ((ptr)) ? (ptr) : ""
 
 static inline int cfs_size_round4(int val)
diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c 
b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
index 70a99cf0..eccfe8bd 100644
--- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
+++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
@@ -98,14 +98,12 @@ int libcfs_ioctl_popdata(void *arg, void *data, int size)
 static int
 libcfs_psdev_open(struct inode *inode, struct file *file)
 {
-   struct libcfs_device_userstate **pdu = NULL;
intrc = 0;
 
if (!inode)
return -EINVAL;
-   pdu = (struct libcfs_device_userstate **)>private_data;
if (libcfs_psdev_ops.p_open != NULL)
-   rc = libcfs_psdev_ops.p_open(0, (void *)pdu);
+   rc = libcfs_psdev_ops.p_open(0, NULL);
else
return -EPERM;
return rc;
@@ -115,14 +113,12 @@ libcfs_psdev_open(struct inode *inode, struct file *file)
 static int
 libcfs_psdev_release(struct inode *inode, struct file *file)
 {
-   struct libcfs_device_userstate *pdu;
intrc = 0;
 
if (!inode)
return -EINVAL;
-   pdu = file->private_data;
if (libcfs_psdev_ops.p_close != NULL)
-   rc = libcfs_psdev_ops.p_close(0, (void *)pdu);
+   rc = libcfs_psdev_ops.p_close(0, NULL);
else
rc = -EPERM;
return rc;
@@ -152,14 +148,8 @@ static long libcfs_ioctl(struct file *file,
return -EPERM;
panic("debugctl-invoked panic");
return 0;
-   case IOC_LIBCFS_MEMHOG:
-   if (!capable(CFS_CAP_SYS_ADMIN))
-   return -EPERM;
-   /* go thought */
}
 
-   pfile.off = 0;
-   pfile.private_data = file->private_data;
if (libcfs_psdev_ops.p_ioctl != NULL)
rc = libcfs_psdev_ops.p_ioctl(, cmd, (void *)arg);
else
diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
b/drivers/staging/lustre/lustre/libcfs/module.c
index 329d78c..0067e53 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -68,142 +68,16 @@ MODULE_LICENSE("GPL");
 
 static struct dentry *lnet_debugfs_root;
 
-static void kportal_memhog_free(struct libcfs_device_userstate *ldu)
-{
-   struct page **level0p = >ldu_memhog_root_page;
-   struct page **level1p;
-   struct page **level2p;
-   intcount1;
-   intcount2;
-
-   if (*level0p != NULL) {
-
-   level1p = (struct page **)page_address(*level0p);
-   count1 = 0;
-
-   while (count1 < PAGE_CACHE_SIZE/sizeof(struct page *) &&
-  *level1p != NULL) {
-
-   level2p = (struct page **)page_address(*level1p);
-   count2 = 0;
-
-   while (count2 < PAGE_CACHE_SIZE/sizeof(struct page *) &&
-  *level2p != NULL) {
-
-   __free_page(*level2p);
-   ldu->ldu_memhog_pages--;
-   level2p++;
-   count2++;
-   }
-
-   __free_page(*level1p);
-   ldu->ldu_memhog_pages--;
-   level1p++;
-   count1++;
-   }
-
-   __free_page(*level0p);
-   ldu->ldu_memhog_pages--;
-
-   *level0p = 

RE: [PATCH 21/40] staging: lustre: improve LNet clean up code and API

[Simmons, James A.]

>Actually we're going to have to redo so much code that it's not worth it
>for me to review the rest of these patches.  

Sorry I didn't get back to you sooner but I was on vacation.  Thanks for 
reviewing this work. Especially since this is the first major bug fixing merge
for the lustre client which means a lot of pain involved to iron out how to
do this. I have been pondering if pushing bug fixes before style cleanups
is the right thing to do. I pushed a bunch of bug fixes earlier and none got
merged which either means Greg is just backed up and hasn't the time to
merge them or  style issues are higher priority. Assuming these bug fixes are
in scope of the staging tree. Should I continue to push this work first?
Well either way I should update this patch series so it ready to merge at some
point.

>Please just look over everything again:
>
> BAD:  return -1;
>GOOD:  return -EINVAL;
>
> BAD:  failed0:
>GOOD:  free_something:
>
> BAD:  if (rc != 0)
>GOOD:  if (rc)
>
>Do one thing per patch.
>Do not introduce a bug and then fix it in a later patch.
>Check ioc_len more carefully.
>Don't make the code look ugly just to please checkpatch.pl.
>Do error handling not success handling.
>Try to avoid indenting a far to the right.

Okay. Will start to do the patch cleanup.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference' errors for LNet

[Simmons, James A.]

>> diff --git a/drivers/staging/lustre/lnet/selftest/conctl.c 
>> b/drivers/staging/lustre/lnet/selftest/conctl.c
>> index 556c837..2ca7d0e 100644
>> --- a/drivers/staging/lustre/lnet/selftest/conctl.c
>> +++ b/drivers/staging/lustre/lnet/selftest/conctl.c
>> @@ -679,45 +679,46 @@ static int
>>  lst_stat_query_ioctl(lstio_stat_args_t *args)
>>  {
>>  int rc;
>> -char *name;
>> +char *name = NULL;
>>  
>>  /* TODO: not finished */
>>  if (args->lstio_sta_key != console_session.ses_key)
>>  return -EACCES;
>>  
>> -if (args->lstio_sta_resultp == NULL ||
>> -(args->lstio_sta_namep  == NULL &&
>> - args->lstio_sta_idsp   == NULL) ||
>> -args->lstio_sta_nmlen <= 0 ||
>> -args->lstio_sta_nmlen > LST_NAME_SIZE)
>> -return -EINVAL;
>> -
>> -if (args->lstio_sta_idsp != NULL &&
>> -args->lstio_sta_count <= 0)
>> +if (!args->lstio_sta_resultp)
>>  return -EINVAL;
>>  
>> -LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> -if (name == NULL)
>> -return -ENOMEM;
>> -
>> -if (copy_from_user(name, args->lstio_sta_namep,
>> -   args->lstio_sta_nmlen)) {
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> -return -EFAULT;
>> -}
>> +if (args->lstio_sta_idsp) {
>> +if (args->lstio_sta_count <= 0)
>> +return -EINVAL;
>>  
>> -if (args->lstio_sta_idsp == NULL) {
>> -rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> -   args->lstio_sta_resultp);
>> -} else {
>>  rc = lstcon_nodes_stat(args->lstio_sta_count,
>> args->lstio_sta_idsp,
>> args->lstio_sta_timeout,
>> args->lstio_sta_resultp);
>> -}
>> +} else if (args->lstio_sta_namep) {
>> +if (args->lstio_sta_nmlen <= 0 ||
>> +args->lstio_sta_nmlen > LST_NAME_SIZE)
>> +return -EINVAL;
>> +
>> +LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> +if (!name)
>> +return -ENOMEM;
>>  
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> +rc = copy_from_user(name, args->lstio_sta_namep,
>> +args->lstio_sta_nmlen);
>> +if (!rc)
>> +rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> +   args->lstio_sta_resultp);
>> +else
>> +rc = -EFAULT;
>>  
>> +} else {
>> +rc = -EINVAL;
>> +}
>> +
>> +if (name)
>> +LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>
>There is no bug fix here.  This code was fine when it was merged into
>the kernel in 2013 so I have no idea how out of date the static checker
>warning is...  The new code doesn't do unnecessary allocations so that's
>good but "name" should be declared in the block where it is used instead
>of at the start of the function.  Btw, we assume that the user gives us
>a NUL terminated string for "name" so we should fix that bug as well.
>
>TODO: lustre: don't assume "name" is NUL terminated

Ugh. I see breakage everywhere in this code :-( Need to address.  I think we
should convert that to strcpy_to_user as well.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

  
>>  struct libcfs_ioctl_hdr {
>>  __u32 ioc_len;
>> @@ -87,6 +88,13 @@ do {  \
>>  data.ioc_hdr.ioc_len = sizeof(data);\
>>  } while (0)
>>  
>> +#define LIBCFS_IOC_INIT_V2(data, hdr)   \
>> +do {\
>> +memset(&(data), 0, sizeof(data));   \
>> +(data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> +(data).hdr.ioc_len = sizeof(data);  \
>> +} while (0)
>> +
>
>Do we really need this?

Would you be okay if this was a inline function? This is used by user land and 
kernel space code.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

>On Tue, Dec 15, 2015 at 06:14:19PM +0000, Simmons, James A. wrote:
>>   
>> >>  struct libcfs_ioctl_hdr {
>> >>   __u32 ioc_len;
>> >> @@ -87,6 +88,13 @@ do {   \
>> >>   data.ioc_hdr.ioc_len = sizeof(data);\
>> >>  } while (0)
>> >>  
>> >> +#define LIBCFS_IOC_INIT_V2(data, hdr)\
>> >> +do { \
>> >> + memset(&(data), 0, sizeof(data));   \
>> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> >> + (data).hdr.ioc_len = sizeof(data);  \
>> >> +} while (0)
>> >> +
>> >
>> >Do we really need this?
>> 
>> Would you be okay if this was a inline function? This is used by user
>> land and kernel space code.
>
>Then your code is broken, please never do that.

This brings up a good point. This header doesn't contain structures for 
userland so it is a uapi
type header.  Should such headers only contain data structures?

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

[Simmons, James A.]

>On Mon, Nov 9, 2015 at 7:07 PM, Michał Kępień  wrote:
>>> Remove the function ll_finish_md_op_data() and replace all its calls
>>> with the standrd function ll_finish_md_op_data().
>>
>> I believe you meant to write "standard function kfree()".
>>
>
>Yes. I am so sorry. Should I be sending the whole series again?
>Thank you
>Shivani

Yes please redo the series. I saw Oleg's concern and I would recommend that
besides the conversion to kfree that you add comments about what is being 
deleted.
I.E

/* Free struct md_op_data  data*/
kfree(...)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] staging: lustre: export cfs_str2mask

[Simmons, James A.]

>> We need cfs_str2mask exported for our server code.
>> Even with the server code not available upstream
>> it would be nice to use the upstream code on Lustre
>> servers.
>> 
>> Signed-off-by: James Simmons 
>> ---
>> .../staging/lustre/lustre/libcfs/libcfs_string.c   |1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c 
>> b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> index d40be53..05630f8 100644
>> --- a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> +++ b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> @@ -111,6 +111,7 @@ int cfs_str2mask(const char *str, const char 
>> *(*bit2str)(int bit),
>>  *oldmask = newmask;
>>  return 0;
>> }
>> +EXPORT_SYMBOL(cfs_str2mask);
>
>If this is the case of it being used out of tree, I suspect a comment here to 
>that effect would be
>useful, otherwise next person running a script to eliminate unused 
>EXPORT_SYMBOLs would kill it again.

I will send another patch with comments not to remove the new code.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] staging: lustre: Handle nodemask on UMP machines

[Simmons, James A.]

>On Sun, Nov 08, 2015 at 11:34:55AM -0500, James Simmons wrote:
>> For UMP and SMP machines the struct cfs_cpt_table are
>> defined differently. In the case handled by this patch
>> nodemask is defined as a integer for the UMP case and
>> as a pointer for the SMP case. This will cause a problem
>> for ost_setup which reads the nodemask directly. Instead
>> we create a UMP version of cfs_cpt_nodemask and use that
>> in ost_setup.
>> 
>> Signed-off-by: James Simmons 
>> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4199
>> Reviewed-on: http://review.whamcloud.com/9219
>> Reviewed-by: Liang Zhen 
>> Reviewed-by: Li Xi 
>> Reviewed-by: Andreas Dilger 
>
>Signed-off-by: and Reviewed-by: tags entered two times. Once here.
 
It is one of those rare situations where two patches are need together.
It would be nice to be able to preserve the reviewers for each one.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] Build Dependency Issue in Lustre Mainline Linux

[Simmons, James A.]

>Just did a fresh git pull --rebase and tried to do 'make deb-pkg' with
>an Ubuntu config (which enables lustre). I got the following:
>
>depmod: WARNING: found 2 modules in dependency cycles!
>depmod: WARNING:
>/home/arges/src/kernel/linux/./debian/tmp/lib/modules/4.3.0+/kernel/drivers/staging/lustre/lnet/lnet/lnet.ko
>in dependency cycle!
>depmod: WARNING:
>/home/arges/src/kernel/linux/./debian/tmp/lib/modules/4.3.0+/kernel/drivers/staging/lustre/lustre/libcfs/libcfs.ko
>in dependency cycle!
>./scripts/depmod.sh: line 57: 16836 Killed  "$DEPMOD"

I solved your problem. If you delete IOC_LIBCFS_PING_TEST ioctl case  from 
libcfs/libcfs/module.c it work again. 
That ioctl has not been is use for some time and it can be removed. What is 
happening is libcfs_nid2str is being
called which requires lnet.ko now. This is a layer violation. I will be sending 
a patch very shortly.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 3/3] Staging: lustre: ldlm_pool: Drop unneeded wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:45 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-ker...@vger.kernel.org
>Subject: [PATCH 3/3] Staging: lustre: ldlm_pool: Drop unneeded wrapper function
>
>Remove the function ldlm_pool_set_limit() and replace its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)
>

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 20cf389..e59b286 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -208,14 +208,6 @@ static inline int ldlm_pool_t2gsp(unsigned int t)
 }
 
 /**
- * Sets passed \a limit to \a pl.
- */
-static void ldlm_pool_set_limit(struct ldlm_pool *pl, __u32 limit)
-{
-   atomic_set(>pl_limit, limit);
-}
-
-/**
  * Recalculates next stats on passed \a pl.
  *
  * \pre ->pl_lock is locked.
@@ -257,7 +249,7 @@ static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
LASSERT(obd != NULL);
read_lock(>obd_pool_lock);
pl->pl_server_lock_volume = obd->obd_pool_slv;
-   ldlm_pool_set_limit(pl, obd->obd_pool_limit);
+   atomic_set(>pl_limit, obd->obd_pool_limit);
read_unlock(>obd_pool_lock);
 }
 
@@ -678,7 +670,7 @@ int ldlm_pool_init(struct ldlm_pool *pl, struct 
ldlm_namespace *ns,
snprintf(pl->pl_name, sizeof(pl->pl_name), "ldlm-pool-%s-%d",
 ldlm_ns_name(ns), idx);
 
-   ldlm_pool_set_limit(pl, 1);
+   atomic_set(>pl_limit, 1);
pl->pl_server_lock_volume = 0;
pl->pl_ops = _cli_pool_ops;
pl->pl_recalc_period = LDLM_POOL_CLI_DEF_RECALC_PERIOD;
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/3] Staging: lustre: ldlm_pool: Remove unneeded wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:43 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-ker...@vger.kernel.org
>Subject: [PATCH 1/3] Staging: lustre: ldlm_pool: Remove unneeded wrapper 
>function
>
>Remove the function ldlm_pl2ns() and replace its calls with the function
>it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 1a4eef6..2beb36b 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -176,11 +176,6 @@ enum {
LDLM_POOL_LAST_STAT
 };
 
-static inline struct ldlm_namespace *ldlm_pl2ns(struct ldlm_pool *pl)
-{
-   return container_of(pl, struct ldlm_namespace, ns_pool);
-}
-
 /**
  * Calculates suggested grant_step in % of available locks for passed
  * \a period. This is later used in grant_plan calculations.
@@ -254,7 +249,8 @@ static void ldlm_pool_recalc_stats(struct ldlm_pool *pl)
 }
 
 /**
- * Sets SLV and Limit from ldlm_pl2ns(pl)->ns_obd tp passed \a pl.
+ * Sets SLV and Limit from container_of(pl, struct ldlm_namespace,
+ * ns_pool)->ns_obd tp passed \a pl.
  */
 static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
 {
@@ -264,7 +260,8 @@ static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
 * Get new SLV and Limit from obd which is updated with coming
 * RPCs.
 */
-   obd = ldlm_pl2ns(pl)->ns_obd;
+   obd = container_of(pl, struct ldlm_namespace,
+  ns_pool)->ns_obd;
LASSERT(obd != NULL);
read_lock(>obd_pool_lock);
pl->pl_server_lock_volume = obd->obd_pool_slv;
@@ -304,7 +301,8 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
/*
 * Do not cancel locks in case lru resize is disabled for this ns.
 */
-   if (!ns_connect_lru_resize(ldlm_pl2ns(pl))) {
+   if (!ns_connect_lru_resize(container_of(pl, struct ldlm_namespace,
+   ns_pool))) {
ret = 0;
goto out;
}
@@ -315,7 +313,8 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
 * It may be called when SLV has changed much, this is why we do not
 * take into account pl->pl_recalc_time here.
 */
-   ret = ldlm_cancel_lru(ldlm_pl2ns(pl), 0, LCF_ASYNC, LDLM_CANCEL_LRUR);
+   ret = ldlm_cancel_lru(container_of(pl, struct ldlm_namespace, ns_pool),
+ 0, LCF_ASYNC, LDLM_CANCEL_LRUR);
 
 out:
spin_lock(>pl_lock);
@@ -341,7 +340,7 @@ static int ldlm_cli_pool_shrink(struct ldlm_pool *pl,
struct ldlm_namespace *ns;
int unused;
 
-   ns = ldlm_pl2ns(pl);
+   ns = container_of(pl, struct ldlm_namespace, ns_pool);
 
/*
 * Do not cancel locks in case lru resize is disabled for this ns.
@@ -558,7 +557,8 @@ static struct kobj_type ldlm_pl_ktype = {
 
 static int ldlm_pool_sysfs_init(struct ldlm_pool *pl)
 {
-   struct ldlm_namespace *ns = ldlm_pl2ns(pl);
+   struct ldlm_namespace *ns = container_of(pl, struct ldlm_namespace,
+ns_pool);
int err;
 
init_completion(>pl_kobj_unregister);
@@ -570,7 +570,8 @@ static int ldlm_pool_sysfs_init(struct ldlm_pool *pl)
 
 static int ldlm_pool_debugfs_init(struct ldlm_pool *pl)
 {
-   struct ldlm_namespace *ns = ldlm_pl2ns(pl);
+   struct ldlm_namespace *ns = container_of(pl, struct ldlm_namespace,
+ns_pool);
struct dentry *debugfs_ns_parent;
struct lprocfs_vars pool_vars[2];
char *var_name = NULL;
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/3] Staging: lustre: ldlm_pool: Drop wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:44 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-ker...@vger.kernel.org
>Subject: [PATCH 2/3] Staging: lustre: ldlm_pool: Drop wrapper function
>
>Remove the function ldlm_pool_get_limit() and replace its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 10 +-
> 1 file changed, 1 insertion(+), 9 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 2beb36b..20cf389 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -208,14 +208,6 @@ static inline int ldlm_pool_t2gsp(unsigned int t)
 }
 
 /**
- * Returns current \a pl limit.
- */
-static __u32 ldlm_pool_get_limit(struct ldlm_pool *pl)
-{
-   return atomic_read(>pl_limit);
-}
-
-/**
  * Sets passed \a limit to \a pl.
  */
 static void ldlm_pool_set_limit(struct ldlm_pool *pl, __u32 limit)
@@ -452,7 +444,7 @@ static int lprocfs_pool_state_seq_show(struct seq_file *m, 
void *unused)
spin_lock(>pl_lock);
slv = pl->pl_server_lock_volume;
clv = pl->pl_client_lock_volume;
-   limit = ldlm_pool_get_limit(pl);
+   limit = atomic_read(>pl_limit);
granted = atomic_read(>pl_granted);
grant_rate = atomic_read(>pl_grant_rate);
cancel_rate = atomic_read(>pl_cancel_rate);
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/3] Staging: lustre: tracefile: Remove wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:19 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-ker...@vger.kernel.org
>Subject: [PATCH 2/3] Staging: lustre: tracefile: Remove wrapper function
>
>Remove the function cfs_trace_free_string_buffer() as it can be replaced
>with the standard function kfree().
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/libcfs/tracefile.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/libcfs/tracefile.c 
b/drivers/staging/lustre/lustre/libcfs/tracefile.c
index d55dda8..211047f 100644
--- a/drivers/staging/lustre/lustre/libcfs/tracefile.c
+++ b/drivers/staging/lustre/lustre/libcfs/tracefile.c
@@ -817,11 +817,6 @@ int cfs_trace_allocate_string_buffer(char **str, int nob)
return 0;
 }
 
-void cfs_trace_free_string_buffer(char *str, int nob)
-{
-   kfree(str);
-}
-
 int cfs_trace_dump_debug_buffer_usrstr(void __user *usr_str, int usr_str_nob)
 {
char *str;
@@ -842,7 +837,7 @@ int cfs_trace_dump_debug_buffer_usrstr(void __user 
*usr_str, int usr_str_nob)
}
rc = cfs_tracefile_dump_all_pages(str);
 out:
-   cfs_trace_free_string_buffer(str, usr_str_nob + 1);
+   kfree(str);
return rc;
 }
 
@@ -898,7 +893,7 @@ int cfs_trace_daemon_command_usrstr(void __user *usr_str, 
int usr_str_nob)
if (rc == 0)
rc = cfs_trace_daemon_command(str);
 
-   cfs_trace_free_string_buffer(str, usr_str_nob + 1);
+   kfree(str);
return rc;
 }
 
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/3] Staging: lustre: module: Replace function calls

[Simmons, James A.]

>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:18 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-ker...@vger.kernel.org
>Subject: [PATCH 1/3] Staging: lustre: module: Replace function calls
>
>Replace the calls of function cfs_trace_free_string_buffer() with
>kfree() as the former function is not required.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/libcfs/module.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
b/drivers/staging/lustre/lustre/libcfs/module.c
index 50e8fd2..516a9e7 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -392,7 +392,7 @@ static int __proc_dobitmasks(void *data, int write,
} else {
rc = cfs_trace_copyin_string(tmpstr, tmpstrlen, buffer, nob);
if (rc < 0) {
-   cfs_trace_free_string_buffer(tmpstr, tmpstrlen);
+   kfree(tmpstr);
return rc;
}
 
@@ -402,7 +402,7 @@ static int __proc_dobitmasks(void *data, int write,
*mask |= D_EMERG;
}
 
-   cfs_trace_free_string_buffer(tmpstr, tmpstrlen);
+   kfree(tmpstr);
return rc;
 }
 
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] Staging: lustre: dir: Remove wrapper function

[Simmons, James A.]

>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 11:55 AM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-ker...@vger.kernel.org; 
>lustre-de...@lists.lustre.org
>Subject: [PATCH] Staging: lustre: dir: Remove wrapper function
>
>Remove the function ll_check_page() and replace all its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/llite/dir.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/llite/dir.c 
b/drivers/staging/lustre/lustre/llite/dir.c
index 5c9502b..842e6d6 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -239,12 +239,6 @@ static int ll_dir_filler(void *_hash, struct page *page0)
return rc;
 }
 
-static void ll_check_page(struct inode *dir, struct page *page)
-{
-   /* XXX: check page format later */
-   SetPageChecked(page);
-}
-
 void ll_release_page(struct page *page, int remove)
 {
kunmap(page);
@@ -432,7 +426,8 @@ struct page *ll_get_dir_page(struct inode *dir, __u64 hash,
goto fail;
}
if (!PageChecked(page))
-   ll_check_page(dir, page);
+   /* XXX: check page format later */
+   SetPageChecked(page);
if (PageError(page)) {
CERROR("page error: "DFID" at %llu: rc %d\n",
   PFID(ll_inode2fid(dir)), hash, -5);
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH] staging: lustre: lnet: klnds: socklnd: Move extern declarations to header

[Simmons, James A.]

>From: lustre-devel [mailto:lustre-devel-boun...@lists.lustre.org] On Behalf Of 
>Amitoj Kaur Chawla
>Sent: Friday, November 06, 2015 9:57 AM
>To: oleg.dro...@intel.com; andreas.dil...@intel.com; 
>gre...@linuxfoundation.org; lustre-de...@lists.lustre.org; 
>de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
>Subject: [lustre-devel] [PATCH] staging: lustre: lnet: klnds: socklnd: Move 
>extern declarations to header
>
>This patch moves extern declarations in socklnd_lib.c to the respective 
>header file, 'socklnd.h'.
>
>This patch also removes extern keyword from function declarations
>since functions have the extern specifier by default.
>
>Signed-off-by: Amitoj Kaur Chawla 
>---
> drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h | 3 +++
> drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c | 2 --
> 2 files changed, 3 insertions(+), 2 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
index b349847..f4fa725 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
@@ -679,6 +679,9 @@ int ksocknal_lib_recv_kiov(ksock_conn_t *conn);
 int ksocknal_lib_get_conn_tunables(ksock_conn_t *conn, int *txmem,
   int *rxmem, int *nagle);
 
+void ksocknal_read_callback(ksock_conn_t *conn);
+void ksocknal_write_callback(ksock_conn_t *conn);
+
 int ksocknal_tunables_init(void);
 
 void ksocknal_lib_csum_tx(ksock_tx_t *tx);
diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
index 679785b..04a4653 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
@@ -580,8 +580,6 @@ ksocknal_lib_push_conn(ksock_conn_t *conn)
ksocknal_connsock_decref(conn);
 }
 
-extern void ksocknal_read_callback(ksock_conn_t *conn);
-extern void ksocknal_write_callback(ksock_conn_t *conn);
 /*
  * socket call back in Linux
  */
-- 
1.9.1

___
lustre-devel mailing list
lustre-de...@lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 07/10] staging: lustre: Handle nodemask on UMP machines

[Simmons, James A.]

>All warnings (new ones prefixed by >>):
>
>   In file included from include/linux/bitops.h:36:0,
>from 
> drivers/staging/lustre/lustre/libcfs/../../include/linux/libcfs/linux/libcfs.h:44,
>from 
> drivers/staging/lustre/lustre/libcfs/../../include/linux/libcfs/libcfs.h:40,
>from drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c:38:
>   drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c: In function 
> 'cfs_cpt_table_alloc':
>>> arch/m68k/include/asm/bitops.h:64:5: warning: passing argument 2 of 
>>> 'bset_mem_set_bit' from incompatible pointer type
>bset_mem_set_bit(nr, vaddr) : \
>^
>>> drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c:61:3: note: in expansion 
>>> of macro 'set_bit'
>  set_bit(0, >ctb_nodemask);

Yep and additional patch exist to fix this. Should I just push the fix for this 
or drop this patch and create
a new patch that is combo of both fixes.

BTW Greg this new batch of patches are order independent. Sorry for not 
pointing that out. The rest of the
patch appear to be okay.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: staging: lustre: provide separate buffers for libcfs_*2str()

[Simmons, James A.]

>Hello Dan,
>
>Thanks to report this.  This issue was fixed in our code but not up streamed 
>yet. The patch is available 
>http://git.whamcloud.com/?p=fs%2Flustre->release.git;a=commit;h=d47f00d5a420b594b49564b2e00efca4602c3fb5

I just pushed a proper patch just a bit ago.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add()

[Simmons, James A.]

>On Tue, Nov 03, 2015 at 05:49:00PM -0600, Frank Zago wrote:
>> Yes, but for consistency, all 4 functions should be changed.
>
>Thanks for the review.  I will send a v2.

Greg merged your original patch so it will be a new patch.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v2 1/7] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>On Mon, Nov 02, 2015 at 12:22:07PM -0500, James Simmons wrote:
>> Cleanup all the unneeded white space in libcfs_hash.h.
>> 
>> Signed-off-by: James Simmons 
>> ---
>>  .../lustre/include/linux/libcfs/libcfs_hash.h  |  135 
>> ++--
>>  1 files changed, 70 insertions(+), 65 deletions(-)
>
>Doesn't apply, did I already queue up this series?

I did a second version of those patches. In one batch you will notice [PATCH 
v2].
The reason I did a second batch was to break up the "fix checkpatch issues" 
patch
in the first series. I was trying to behave :-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH v2 1/7] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>On Tue, Nov 03, 2015 at 07:46:07PM -0800, Greg Kroah-Hartman wrote:
>> On Mon, Nov 02, 2015 at 12:22:07PM -0500, James Simmons wrote:
>> > Cleanup all the unneeded white space in libcfs_hash.h.
>> > 
>> > Signed-off-by: James Simmons 
>> > ---
>> >  .../lustre/include/linux/libcfs/libcfs_hash.h  |  135 
>> > ++--
>> >  1 files changed, 70 insertions(+), 65 deletions(-)
>> 
>> Doesn't apply, did I already queue up this series?
>
>No. This did not apply because of:
>c7fdf4a3959f ("staging: lustre: fix remaining checkpatch issues for 
>libcfs_hash.h")

Surprise this was merged which is why I did a second series for this.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [patch] staging: lustre: remove unused variable

[Simmons, James A.]

>The "->lr_lvb_data" struct member is never used.  Delete it.
>
>Signed-off-by: Dan Carpenter 
>---
>I have compile tested this.  I looked through the history and it seems
>to never have been used.  I wonder if we can remove the locking as well.

This is used by the quota and ofd layer which are both server side. 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: staging: lustre: provide separate buffers for libcfs_*2str()

[Simmons, James A.]

>Hello Dmitry Eremin,
>
>This is a semi-automatic email about new static checker warnings.
>
>The patch 80feb1ef349e: "staging: lustre: provide separate buffers 
>for libcfs_*2str()" from Oct 21, 2015, leads to the following Smatch 
>complaint:

I created a ticket for this at https://jira.hpdd.intel.com/browse/LU-7380.
A fix should be created shortly and run by the test suite.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

[Simmons, James A.]

>Yeah.  That is often the fastest way to fix all the checkpatch warnings.
>
>Checkpatch warnings are pretty mechanical.  Just send like 100 patches
>at a time until everything is fixed.  Don't overthink.  Say your patch
>breaks the alignment then you have to fix that, but otherwise only fix
>one thing at a time.  Sometimes people will ask you to fix something
>else on the same line, but just say "I didn't introduce that, but yes I
>am planning to fix that in a later patchset since I am following the
>one thing per patch rule."
>
>Don't feel shame about sending many small patches.  We pretty much merge
>everything.

It was the sense of it taking forever with that amount of patches needed with
the one file approach. Looking at the back log of fixes its not as bad as I 
thought
for libcfs/LNet. Once those fixes are merged the style cleanups can happen
pretty quickly.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [patch] staging: lustre: potential underflow in libcfs_kkuc_group_add()

[Simmons, James A.]

>My static checker says that "group" is a user controlled number that can
>be negative leading to an array underflow.  I have looked at it, and I'm
>not an expert enough in lustre to say for sure if it is really a bug.
>Anyway, it's simple enough to make the variable unsigned which pleases
>the static checker and makes it easier to audit.
>
>Signed-off-by: Dan Carpenter 

Dmitry do you this this could be placed under place under LU-6303 or does a new 
ticket need to be open.
I also CC Frank Zago who is very familiar with this code. To me it looks okay.

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h 
b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
index a989d26..41f3d81 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_kernelcomm.h
@@ -91,7 +91,7 @@ typedef int (*libcfs_kkuc_cb_t)(__u32 data, void *cb_arg);
 /* Kernel methods */
 int libcfs_kkuc_msg_put(struct file *fp, void *payload);
 int libcfs_kkuc_group_put(int group, void *payload);
-int libcfs_kkuc_group_add(struct file *fp, int uid, int group,
+int libcfs_kkuc_group_add(struct file *fp, int uid, unsigned int group,
 __u32 data);
 int libcfs_kkuc_group_rem(int uid, int group);
 int libcfs_kkuc_group_foreach(int group, libcfs_kkuc_cb_t cb_func,
diff --git a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c 
b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
index ad661a3..d8230ae 100644
--- a/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
+++ b/drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
@@ -110,7 +110,8 @@ static DECLARE_RWSEM(kg_sem);
  * @param uid identifier for this receiver
  * @param group group number
  */
-int libcfs_kkuc_group_add(struct file *filp, int uid, int group, __u32 data)
+int libcfs_kkuc_group_add(struct file *filp, int uid, unsigned int group,
+ __u32 data)
 {
struct kkuc_reg *reg;
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging:lustre: Prevent duplicate CT registrations

[Simmons, James A.]

>On Fri, Oct 23, 2015 at 03:59:14PM -0400, James Simmons wrote:
>> diff --git a/drivers/staging/lustre/lustre/lmv/lmv_obd.c 
>> b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> index 635a93c..d6d70d8 100644
>> --- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> +++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> @@ -794,7 +794,9 @@ static void lmv_hsm_req_build(struct lmv_obd *lmv,
>>  static int lmv_hsm_ct_unregister(struct lmv_obd *lmv, unsigned int cmd, int 
>> len,
>>   struct lustre_kernelcomm *lk, void *uarg)
>>  {
>> -int i, rc = 0;
>> +struct kkuc_ct_data *kcd = NULL;
>> +int rc = 0;
>> +__u32 i;
>
>We have been introducing a lot of new __u32 types here and I just
>assumed there was a reason for it but this one is clearly wrong.  The
>new code implies that ->ld_tgt_count can overflow INT_MAX which is not
>true and that this is code shared with userspace which might be true but
>it's not described in the changelog.  Is this a static checker fix?
>Stop using that broken static checker, because the correct type here is
>int.

No this patch is from a real fix. You can read the details at
https://jira.hpdd.intel.com/browse/LU-3882. I bet he did this to avoid
the pickiness of gcc. We have build failure due to gcc stupidity. I could
be wrong which if that is the case the original Author is CC.

>Anyway, stop making gratuitous unrelated changes (like the white space
>changes to local declarations).  I feel like I have held off commenting
>on this for a while and shown great restraint.  :P

This is a direct drop of the original patch. I see lots of corrections to the
code below. Would a new follow on patch to correct these issues be okay
with you. This way we can perverse our bug fix history.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

[Simmons, James A.]

>On Thu, Oct 29, 2015 at 07:28:21PM -0400, James Simmons wrote:
>> With nidstring now having the latest fixes we can
>> now clean up all the remaining checkpatch errors
>> for nidstring.c.
>
>Please be specific as to exactly what you changed, and break it up into
>one-patch-per-thing.  And no, "fix all checkpatch errors" is not "one
>thing"

Hmm. This makes me think I might be going about this wrong.  Instead of
doing style changes per file I should be doing one style change per subsystem
instead. Unless you prefer doing these style changes on per file base. Perhaps
for now I should focus on pushing the fixes that have cumulated and once
caught up then finished off the style issues. 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 09/10] staging: lustre: fix remaining checkpatch issues for libcfs_hash.h

[Simmons, James A.]

>>diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>b/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>index 5df8ba2..563b2b4 100644
>>--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>@@ -62,7 +62,8 @@
>> /** disable debug */
>> #define CFS_HASH_DEBUG_NONE  0
>> /** record hash depth and output to console when it's too deep,
>>- *  computing overhead is low but consume more memory */
>>+ *  computing overhead is low but consume more memory
>>+ */
>
>Typically, multi-line comments have the leading /* on a separate line
>from the first line of text.  If you are changing all these comments
>you may as well make it consistent with the kernel style.

Fixed for next patch series.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 08/10] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>>struct cfs_hash_bd {
>>- struct cfs_hash_bucket  *bd_bucket;  /**< address of bucket */
>>- unsigned intbd_offset;  /**< offset in bucket */
>>+ /**< address of bucket */
>>+ struct cfs_hash_bucket  *bd_bucket;
>>+ /**< offset in bucket */
>>+ unsigned int bd_offset;
>> };
>
>The "/**< ... */" marker means "the field to the left", but if you are
>moving these to the line before the field you should just use "/* ... */".

Fixed.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 5/5] staging:lustre: Update license and copyright for kernel comm

[Simmons, James A.]

>>
>> - * http://www.sun.com/software/products/lustre/docs/GPLv2.pdf
>> - *
>> - * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
>> - * CA 95054 USA or visit www.sun.com if you need additional information or
>> - * have any questions.
>
>That text is in every files. You should kill all references.

That would be a mighty big patch.  A patch already exist in our development 
branch
to update the copyright so that will be eventually backported to the staging 
tree.
I can look into updating the rest after the copyright patch lands. Then I will 
port to the
staging tree.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/2] Staging: lustre: lnet: lib-move return of an errno should typically be negative (ie: return -EAGAIN)

[Simmons, James A.]

>Fixed- Return of an errno should typically be negative (ie: return -EAGAIN)
>
>Signed-off-by: Nilesh Kokane 
>---
> drivers/staging/lustre/lnet/lnet/lib-move.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)

Bad idea. I also made this mistake and it broke LNet really badly.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] LIBCFS_ALLOC

[Simmons, James A.]

 Yeah.  You're right.  Doing a vmalloc() when kmalloc() doesn't have even
 a tiny sliver of RAM isn't going to work.  It's easier to use
 libcfs_kvzalloc() everywhere, but it's probably the wrong thing.

 The original  reason we have the vmalloc water mark wasn't so much the
 issue of memory exhaustion but to handle the case of memory fragmentation.
 Some sites had after a extended period of time started to see failures of
 allocating even 32K using kmalloc.  In our latest development branch we moved
 away from using a water mark to always try kmalloc first and if it fails 
 then we
 try vmalloc. At ORNL we ran into severe performance issues when we entered
 vmalloc territory. It has been discussed before on what might replace vmalloc
 handling in the case of kmalloc fails but no solution has been worked out.

OK, but if a structure contains only 4 words, would it be better to just
use kzalloc?  Or does it not matter?  It would only save trying vmalloc in
a case that it is guaranteed to fail, but if a structure with 4 words
can't be allocated, the system has other problems.  Another argument is
that kzalloc is a well known function that people and bug-finding tools
understand, so it is better to use it whenever possible.

Some of the other structures contain a lot more fields, as well as small
arrays.  They are probably acceptable for kzalloc too, but I wouldn't know
the exact dividing line.

The reason I bring this up is to discuss sorting this out. Once long ago we had
just  LIBCFS_ALLOC. For some reason before my time OBD_ALLOC got spawned
off of that.  Currently LIBCFS_ALLOC is used just by the libcfs/LNet layer. Now 
OBD_ALLOC in our development branch has moved to a try kmalloc first and 
if it fails try vmalloc for any size memory allocation.  LIBCFS_ALLOC still 
does the
original approach.   So we have two possible solutions depending on if 
libcfs/LNet
needs to ever do a vmalloc.

One solution if libcfs/LNet never needs a vmalloc is remove LIBCFS_ALLOC and 
replace
it with kzalloc everywhere. We can then move libcfs_kzvalloc to the lustre 
layer and
port the change we did in the development branch to here of the try kmalloc then
vmalloc approach. 

The other approach is if libcfs/LNet does in some case need to use vmalloc we 
could 
then update LIBCFS_ALLOC to first try kmalloc then vmalloc. Once this is 
implemented
we can nuke the OBD_ALLOC system.

Either way I like to see it consolidated down to one system.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: LIBCFS_ALLOC

[Simmons, James A.]

Yeah.  You're right.  Doing a vmalloc() when kmalloc() doesn't have even
a tiny sliver of RAM isn't going to work.  It's easier to use
libcfs_kvzalloc() everywhere, but it's probably the wrong thing.

The original  reason we have the vmalloc water mark wasn't so much the
issue of memory exhaustion but to handle the case of memory fragmentation. 
Some sites had after a extended period of time started to see failures of
allocating even 32K using kmalloc.  In our latest development branch we moved
away from using a water mark to always try kmalloc first and if it fails then we
try vmalloc. At ORNL we ran into severe performance issues when we entered
vmalloc territory. It has been discussed before on what might replace vmalloc
handling in the case of kmalloc fails but no solution has been worked out.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 4/9] staging:lustre: merge socklnd_lib-linux.h into socklnd.h

[Simmons, James A.]

  diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h 
  b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
  index 53275f9..7125eb9 100644
  --- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
  +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
  @@ -25,16 +25,40 @@
*
*/
 
  +#ifndef _SOCKLND_SOCKLND_H_
  +#define _SOCKLND_SOCKLND_H_
  +
   #define DEBUG_PORTAL_ALLOC
   #define DEBUG_SUBSYSTEM S_LND
 
  -#include socklnd_lib-linux.h
  +#include asm/irq.h
  +#include linux/crc32.h
  +#include linux/errno.h
 
 Including asm/irq.h first causes a build failure for m68k/allmodconfig:
 
 arch/m68k/include/asm/irq.h:77:12: error: expected '=', ',', ';',
 'asm' or '__attribute__' before 'void'
 arch/m68k/include/asm/irq.h:78:1: error: unknown type name 'atomic_t'
 arch/m68k/include/asm/irq.h:77:12: error: expected '=', ',', ';',
 'asm' or '__attribute__' before 'void'
 arch/m68k/include/asm/irq.h:78:1: error: unknown type name 'atomic_t'
 
 http://kisskb.ellerman.id.au/kisskb/buildresult/12448922/
 
 Fixing it inside arch/m68k/include/asm/irq.h might cause Include Hell,
 so perhaps you can just move the asm/* include below all linux/*
 includes?

I looked at our main development branch and I see socklnd.h no longer has
#include asm/irq.h. We can just remove the irq.h from socklnd.h.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v4 4/9] staging:lustre: merge socklnd_lib-linux.h into socklnd.h

[Simmons, James A.]

On Thu, Jun 25, 2015 at 3:33 AM, Guenter Roeck li...@roeck-us.net wrote:
 I have not tested it, but I think the following may fix the problem
 while avoiding any include problems. Since pt_regs is used in the file,
 one could argue that it should be declared.

Indeed. I tried that, but...

 --
 diff --git a/arch/m68k/include/asm/irq.h b/arch/m68k/include/asm/irq.h
 index 81ca118d58af..28ffa8d59cf0 100644
 --- a/arch/m68k/include/asm/irq.h
 +++ b/arch/m68k/include/asm/irq.h
 @@ -74,6 +74,8 @@ extern unsigned int irq_canonicalize(unsigned int irq);
  #define irq_canonicalize(irq)  (irq)
  #endif /* !(CONFIG_M68020 || CONFIG_M68030 || CONFIG_M68040 || 
 CONFIG_M68060) */

 +struct pt_regs;
 +
  asmlinkage void do_IRQ(int irq, struct pt_regs *regs);
  extern atomic_t irq_err_count;

... asmlinkage and atomic_t are also needed.

I didn't want to risk introducing more breakage by adding (at least) three
more includes.

Hi Geert

  Long time. I agree the above is not the best approach. Lets fix lustre 
instead.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure

[Simmons, James A.]

 Yes.  I know Al's thoughts and kernel style.

 But Alan Cox and Andreas have both said they think (x == NULL) can help
 you avoid some kind of boolean vs pointer bugs.  I've had co-workers who
 did massive seds changing !foo to foo == NULL on our code base.  But
 I've never seen a real life example of a bug this fixes.

 To be honest, I've never seen a real life proof that (!foo) code is less
 buggy.  I should look through the kbuild mailbox...  Hm...  But my other
 idea of setting up code style readability testing website is also a good
 one.

 Linux kernel style is based on Joe Perches finding that 80% of the code
 prefers one way or the other.  That's a valid method for determining
 code style.  I bet it normally picks the more readable style but it
 would be interesting to measure it more formally.

On today's linux-next, I find 3218 tests on the result of kmalloc etc
using NULL and 14429 without, making 82% without.  The complete semantic
patch is shown below.

Most people doing something a certain way is not a technical argument. Usually
people do what they are taught. From most people's comments their seems to 
be no technical reason to us one over another. I do have one technical reason 
not
to accept these patches. It is too easy to make a mistake and break things very 
badly.
I don't think it is worth the risk for a non-hard requirement.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Simmons, James A.]

On Wed, Jun 10, 2015 at 5:48 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:

 Are you sure all of these are correct?  The kernel/user api for lustre
 is a complex beast, and just casting away the pointer types isn't
 usually the proper thing to do in order to resolve the issues here.

 thanks,

 greg k-h

I'm not 100% sure, but the pointers that I added the annotation to end
up being used as user memory. (eg. passed to copy_to_user, etc.)
Sometimes these pointers are passed to functions that already have
__user annotation in their signatures (eg. ll_getname, copy_and_ioctl,
ll_fid2path, etc.).

Using these simple cast are not the proper fix. We had a lot of issues with user
land tools breaking due to leakage of kernel space stuff and other problems.
Some work went into cleaning that up in the OpenSFS branch but it is not totally
complete yet. Evans you wanted something challenging to work on well this is
up your alley. I would recommend looking at JIRA ticket LU-6401 and all its sub
tickets.  You could start the port of those to the upstream client. At the same
time we can finish the cleanup in the OpenSFS branch as well.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH v3 7/8] staging:lustre: style cleanups for LNet headers

[Simmons, James A.]

We're going through and re-indenting things.  I think just one space
between type and name is the right thing for .c files but you guys
really should figure out what style you're using for your header files.

This is good to know. I didn't know how you felt about the lustre style.
I will fix it up in the next patch.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH v3 5/8] staging:lustre: separate kernel and user land defines in the LNet headers

[Simmons, James A.]

On 2015/06/05, 3:02 AM, Dan Carpenter dan.carpen...@oracle.com wrote:

On Wed, Jun 03, 2015 at 04:43:24PM -0400, James Simmons wrote:
 Currently the lnet headers used by user land contain various internal
 LNet structures that are only used by kernel space. Move the user land
 structures to headers used by user land. The kernel structures are
 relocated to headers that are never exposed to user land.
 
 Signed-off-by: James Simmons jsimm...@infradead.org
 ---
 diff --git a/drivers/staging/lustre/lnet/lnet/acceptor.c
b/drivers/staging/lustre/lnet/lnet/acceptor.c
 index 1dc7c8a..4928f5c 100644
 --- a/drivers/staging/lustre/lnet/lnet/acceptor.c
 +++ b/drivers/staging/lustre/lnet/lnet/acceptor.c
 @@ -243,8 +243,6 @@ lnet_accept(struct socket *sock, __u32 magic)
  
 if (magic == le32_to_cpu(LNET_PROTO_TCP_MAGIC))
 str = 'old' socknal/tcpnal;
 -   else if (lnet_accept_magic(magic, LNET_PROTO_RA_MAGIC))
 -   str = 'old' ranal;
 else
 str = unrecognised;
  

Presumably this was done intentionally.  We deleted LNET_PROTO_RA_MAGIC.
The changelog was not very clear why.

The Rapid Array network interface is an obsolete network once used by
Cray hardware but hasn't been supported for about 10 years and was (mostly)
deleted from the tree already.  This was just a left-over I guess.

I need to break this patch up and do a proper backport of the patch from 
LU-6209.
Currently it is mixed in with this header cleanup. 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH v3 6/7] staging:lustre: style cleanups for lib-socket.c

[Simmons, James A.]

On Wed, 2015-06-03 at 10:32 -0400, James Simmons wrote:
 Handle all the style issues reported by checkpatch.pl.
 Remove general white spaces, spaces in function calls,
 etc.
[]
 @@ -167,13 +164,14 @@ lnet_ipif_enumerate (char ***namesp)
  if (nalloc * sizeof(*ifr)  PAGE_CACHE_SIZE) {
  toobig = 1;
  nalloc = PAGE_CACHE_SIZE/sizeof(*ifr);
 -CWARN(Too many interfaces: only enumerating first 
 %d\n,
 -  nalloc);
 +CWARN(Too many interfaces: only enumerating 
 +  first %d\n, nalloc);

Please don't split single strings into multiple parts.

For a patch like this it'd be nice to specify that both
git diff -w and scripts/objdiff shows no differences.

Unless it does...

Two patches have mistakes. Should I send a new series or do a in-reply instead.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH v3 4/7] staging:lustre: rename tcpip handling functions to lnet_* prefix

[Simmons, James A.]

On Wed, Jun 03, 2015 at 10:32:31AM -0400, James Simmons wrote:
 With all the TCPIP handling done in the lnet layer we should
 rename all the functions with the prefix lnet_*. One other
 change done was changing the remove argument of lnet_sock_getaddr
 from a int to a bool.
 
 Signed-off-by: James Simmons jsimmons at infradead.org
checkpatch is complaining about unrecognized email address.

For some reason git format will mangle some of email addresses on me.
Any idea why it does that?

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH v1] staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

[Simmons, James A.]

On Mon, Jun 01, 2015 at 12:21:30PM -0700, David Decotigny wrote:
 Thanks for reviewing.
 
 The 2 struct members were not marked as __user, which this patch does
 here. This was causing warnings with copy from/to user (see commit
 description). This patch also propagates the annotation to the couple
 of functions that are using those members.

Lustre's structures are a total mess of kernel and user pointers and
trying to properly mark them as which they are supposed to be at what
point in time is a very difficult task.  People keep trying and get it
wrong, so I suggest just leaving this alone until the developers unwind
the structure mess as that will be necessary for this code to get merged
into the main part of the kernel.

Greg is right. The earlier patch set I sent out for the LNet headers
address this issue for the LNet layer. I also having patches coming that
fix libcfs ioctl handling as well. I see Shuey's patches made it in first so
I'm going to have to rebase. I will send out the new patch sets later today.
This will be v3 of the patch set.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 5/6] staging:lustre: style cleanups for lib-socket.c

[Simmons, James A.]

On Fri, May 22, 2015 at 02:32:31PM -0400, James Simmons wrote:
 @@ -167,13 +164,14 @@ lnet_ipif_enumerate (char ***namesp)
  if (nalloc * sizeof(*ifr)  PAGE_CACHE_SIZE) {
  toobig = 1;
  nalloc = PAGE_CACHE_SIZE/sizeof(*ifr);
 -CWARN(Too many interfaces: only enumerating first 
 %d\n,
 -  nalloc);
 +CWARN(Too many interfaces: only enumerating 
 +  first %d\n, nalloc);
  }

Don't split string literals, it makes them hard to grep for.

Will fix. The CWARN will go over 80 characters but from the recent emails that 
is more acceptable.
If this is the only problem then this patch set it ready.  I have more patch 
series that are dependent
on this first one. Should I push the other patch series with a note that it is 
dependent on the tcpip
cleanup or wait until it is merged? Also how does one find out when the patch 
has been merged?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [PATCH 5/6] staging:lustre: style cleanups for lib-socket.c

[Simmons, James A.]

 
 Don't split string literals, it makes them hard to grep for.
 
 Will fix. The CWARN will go over 80 characters but from the recent emails 
 that is more acceptable.
 If this is the only problem then this patch set it ready.

Normally the right thing to do here would be to send a fixed
[patch 5/6 v2] using the --in-reply-to option so that it appears as a
reply to the original [patch 5/6].

Made a note of that for future reference. 

 I have more patch series that are dependent
 on this first one. Should I push the other patch series with a note that it 
 is dependent on the tcpip
 cleanup or wait until it is merged? Also how does one find out when the 
 patch has been merged?

You will get an email when these are merged.

This is the only issue, I had.  No one else has complained so that means
no one else has any objections.  Greg hasn't merged it yet and he might
find a problem with it, but it seems like a straight forward patchset
so that's unlikely.

Ugh. I was off by one for the number of patches so I need to send a new batch.

The only issue is that this patchset was sent in a confusing way.  It
doesn't have a v2 tag and it was tacked on to the old thread.  Greg
tends to not waste time being confused and just deletes the whole thread
when that happens.

Forgot to change the patch tag number. I will send a new batch with v2 so it
is a new thread.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 2/11] Staging: lustre: fld: Use kzalloc and kfree

[Simmons, James A.]

  When is soon?  How about, if I don't see some real work happening from
  you all in the next 2 months (i.e. before 4.1-final), I drop lustre from
  the tree in 4.2-rc1.  Given that you all have had over 2 years to get
  your act together, and nothing has happened, I think I've been waiting
  long enough, don't you?

As you see from a earlier email from Oleg work is being done to change things.

 I agree we've been much slower in doing a bunch of requested cleanups than 
 initially
 hoped for variety of reasons, not all of which are under our direct control.
 
 Still, please don't drop Lustre client from the staging tree. People seem to 
 be
 actively using that port too (on smaller scale) and we'll improve the 
 cleanups
 situation.

much slower?  Seriously?  It would take one junior developer a month
tops to clean up all of the obvious issues with the in-kernel code, so
that you could then tackle the real issues.  A good developer could do
it all in a single week.  As that's obviously not going to ever happen,
I have no choice but to delete the code from the kernel tree as no one
is working to get it out of staging at all.

Also, having it in the tree is wasting core kernel developer's time and
energy trying to work around things in your codebase.

Since I'm just starting to get involved in this work I'm not aware of the
task you are looking for. What needs to be done from your perspective?
One of the things I have discussed with other developers is the idea
of breaking the cleanup into two stages. First is bringing  libcfs/lnet
up to date and synced to the upstream standards. This is due to lustre
being a application of LNet. LNet is used by various vendors for other
purposes. If this is acceptable to you it can be started right away.  Please
send of list of task that needs to be done for libcfs/lnet work.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 2/11] Staging: lustre: fld: Use kzalloc and kfree

[Simmons, James A.]

 Yes the LARGE functions do the switching. I was expecting also patches to 
 remove the 
 OBD_ALLOC_LARGE functions as well which is not the case here.  I do have one 
 question still. The
 macro __OBD_MALLOC_VERBOSE allowed the ability to simulate memory allocation 
 failures at
 a certain percentage rate. Does something exist in the kernel to duplicate 
 that functionality?
 Once these macros are gone we lose the ability to simulate high memory 
 allocation failures.

Yes, there are things like https://lkml.org/lkml/2014/12/25/64
So I think the API is even riher compared to what our old wrapper code was 
able to do.

We should look to integrating that into the test suite.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 2/11] Staging: lustre: fld: Use kzalloc and kfree

[Simmons, James A.]

 Yes the LARGE functions do the switching. I was expecting also patches to 
 remove the 
 OBD_ALLOC_LARGE functions as well which is not the case here.  I do have one 
 question still. The
 macro __OBD_MALLOC_VERBOSE allowed the ability to simulate memory allocation 
 failures at
 a certain percentage rate. Does something exist in the kernel to duplicate 
 that functionality?

Yes, no need for lustre to duplicate yet-another-thing the kernel
already provides :)

The reason for this is that libcfs was written 10+ years ago which was before 
linux had such nice
features. At that time it was needed to fill the gaps missing which is no 
longer the case. Libcfs is
really showing its age :-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 2/11] Staging: lustre: fld: Use kzalloc and kfree

[Simmons, James A.]

 We are hopefully going to get rid of OBD_ALLOC_LARGE() as well, though.
 
 It's simple enough to write a function:
 
 void *obd_zalloc(size_t size)
 {
 if (size  4 * PAGE_CACHE_SIZE)
 return vzalloc(size);
 else
 return kmalloc(size, GFP_NOFS);
 }
 
 Except, huh?  Shouldn't we be using GFP_NOFS for the vzalloc() side?
 There was some discussion of that GFP_NOFS was a bit buggy back in 2010
 (http://marc.info/?l=linux-mmm=128942194520631w=4) but the current
 lustre code doesn't try to pass GFP_NOFS.
 
 The version in the upstream client is out of date. The current macro in the 
 Intel master
 Branch is:

That's not helpful at all, why do we even have an in-kernel version of
this code if you don't do your development in the kernel?

Please sync with the kernel tree very soon, or I'm just going to delete
this whole thing.  This is getting _really_ frustrating.

First I want to make it clear I am here to help clean up the upstream client. I 
agree in the long run
it is important to move the development to what is in the upstream kernel but 
their is reason why
current development is not focus in the upstream client. 

   As the primary engineer responsible for the deployment of Lustre at ORNL 
I have to ensure
Lustre runs flawlessly. There is zero tolerance of problems or even the 
slightest performance
degradation. Trust me the users scream even when 1% performance is lost. The 
amazing 
thing is we have less then 1% down time during the year. To do this I have to 
perform hundreds
of hours of testing at various scales for various versions of Lustre. This 
includes taking time on Titan.
So what does this have to do with upstream testing. Well no super computer in 
the world runs the 
latest and greatest linux kernel so the focus is just not there. Luckily the 
lab does see it is in its interest
to support the upstream client work otherwise I wouldn't be here :-)
  Second and far more importantly the upstream lustre code currently does 
not have the same 
level of QA with what the Intel branch gets.  The bar is very very high to get 
any patch merged for the
Intel branch. Each patch has to first pass a regression test suite besides the 
normal review process.
Besides that sites like ORNL have to evaluated all the changes at all the 
scales present on site. This
means doing testing on Titan because unique problems only show up at that 
scale. Because of this
the work that will soon come your way has to be first evaluated on the Intel 
branch since this is the
current path for QA. You can think of the intel branch as a lustre-next branch 
that needs to be feed back
too your branch. Eventually your branch will have to under go this level of QA 
but we are not quite
their yet.

 Now I like to see the current situation change and Greg you have know me 
for a while so you 
can expect a lot of changes are coming.  In fact I already have rallied people 
from vendors outside Intel
as well as universities which have done some excellent work which you will soon 
see. Now I hope this
is the last email I do like this. Instead I just want to send you patches. Greg 
I think the changes you will
see soon will remove your frustration.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 2/11] Staging: lustre: fld: Use kzalloc and kfree

[Simmons, James A.]

From: Julia Lawall julia.law...@lip6.fr

Replace OBD_ALLOC, OBD_ALLOC_WAIT, OBD_ALLOC_PTR, and OBD_ALLOC_PTR_WAIT by
kalloc/kcalloc, and OBD_FREE and OBD_FREE_PTR by kfree.

Nak: James Simmons jsimm...@infradead.org

A simple replace will not work. The OBD_ALLOC and OBD_FREE functions allocate 
memory
anywhere from one page to 4MB in size. You can't use kmalloc for the 4MB 
allocations.
Currently lustre uses  a 4 page water mark to determine if we allocate using 
vmalloc. Even
using kmalloc for 4 pages has shown high failure rates on some systems. It gets 
even more
messy with 64K page systems like ppc64 boxes. Now I'm not suggesting to port 
the larger
allocations to vmalloc either since issues have been founded with using 
vmalloc. For example
when using large stripe count files the MDS rpc generated crosses the 4 page 
line and vmalloc
is used. Using vmalloc caused a global spinlock to be taken which causes meta 
data operations
to serialized on the MDS servers.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 2/11] Staging: lustre: fld: Use kzalloc and kfree

[Simmons, James A.]

We are hopefully going to get rid of OBD_ALLOC_LARGE() as well, though.

It's simple enough to write a function:

void *obd_zalloc(size_t size)
{
   if (size  4 * PAGE_CACHE_SIZE)
   return vzalloc(size);
   else
   return kmalloc(size, GFP_NOFS);
}

Except, huh?  Shouldn't we be using GFP_NOFS for the vzalloc() side?
There was some discussion of that GFP_NOFS was a bit buggy back in 2010
(http://marc.info/?l=linux-mmm=128942194520631w=4) but the current
lustre code doesn't try to pass GFP_NOFS.

The version in the upstream client is out of date. The current macro in the 
Intel master
Branch is:

#define __OBD_VMALLOC_VERBOSE(ptr, cptab, cpt, size)  \
do {  \
(ptr) = cptab == NULL ?   \
__vmalloc(size, GFP_NOFS | __GFP_HIGHMEM | __GFP_ZERO,\
  PAGE_KERNEL) :  \
cfs_cpt_vzalloc(cptab, cpt, size);\
if (unlikely((ptr) == NULL)) {\
CERROR(vmalloc of ' #ptr ' (%d bytes) failed\n,   \
   (int)(size));  \
CERROR(LPU64 total bytes allocated by Lustre, %d by LNET\n, \
   obd_memory_sum(), atomic_read(libcfs_kmemory));   \
} else {  \
OBD_ALLOC_POST(ptr, size, vmalloced);   \
} \
} while(0)
 
Then it's simple enough to change OBD_FREE_LARGE() to kvfree().

Also it's weird that only the lustre people have thought of this trick
to allocate big chunks of RAM and no one else has.  What would happen if
we just change vmalloc() so it worked this way for everyone?

Do we really want to encourage vmalloc usages?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [HPDD-discuss] [PATCH 2/11] Staging: lustre: fld: Use kzalloc and kfree

[Simmons, James A.]

 From: Julia Lawall julia.law...@lip6.fr
 
 Replace OBD_ALLOC, OBD_ALLOC_WAIT, OBD_ALLOC_PTR, and OBD_ALLOC_PTR_WAIT by
 kalloc/kcalloc, and OBD_FREE and OBD_FREE_PTR by kfree.
 
 Nak: James Simmons jsimm...@infradead.org
 
 A simple replace will not work. The OBD_ALLOC and OBD_FREE functions 
 allocate memory
 anywhere from one page to 4MB in size. You can't use kmalloc for the 4MB 
 allocations.
 Currently lustre uses  a 4 page water mark to determine if we allocate using 
 vmalloc. Even
 using kmalloc for 4 pages has shown high failure rates on some systems. It 
 gets even more
 messy with 64K page systems like ppc64 boxes. Now I'm not suggesting to port 
 the larger
 allocations to vmalloc either since issues have been founded with using 
 vmalloc. For example
 when using large stripe count files the MDS rpc generated crosses the 4 page 
 line and vmalloc
 is used. Using vmalloc caused a global spinlock to be taken which causes 
 meta data operations
 to serialized on the MDS servers.

It's not the LARGE functions that do the switching?  For example OBD_ALLOC 
ends up at  __OBD_MALLOC_VERBOSE, which as far as I can see calls kmalloc 
(with __GFP_ZERO, and hance the use of kzalloc).

Yes the LARGE functions do the switching. I was expecting also patches to 
remove the 
OBD_ALLOC_LARGE functions as well which is not the case here.  I do have one 
question still. The
macro __OBD_MALLOC_VERBOSE allowed the ability to simulate memory allocation 
failures at
a certain percentage rate. Does something exist in the kernel to duplicate that 
functionality?
Once these macros are gone we lose the ability to simulate high memory 
allocation failures.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel