RE: [HPDD-discuss] [PATCH] staging: lustre: lustre: include: lustre_update.h: Fix for possible null pointer dereference
Ashley, I sort of understand your larger point, but in this case, I think the purpose of the assert is much better served by the move Rickard is suggesting. Otherwise only one of its conditions will ever trigger. It's not that different to die on the assertion or from a null dereference, but I think it's marginally better to fail the assertion. And it definitely doesn't make sense to have it there and never triggered, which it was before. - Patrick From: HPDD-discuss [hpdd-discuss-boun...@lists.01.org] on behalf of Ashley Pittman [apitt...@ddn.com] Sent: Friday, January 02, 2015 11:55 AM To: Rickard Strandqvist Cc: de...@driverdev.osuosl.org; hpdd-disc...@lists.01.org; Greg Kroah-Hartman; linux-kernel@vger.kernel.org Subject: Re: [HPDD-discuss] [PATCH] staging: lustre: lustre:include: lustre_update.h: Fix for possible null pointer dereference Rickard, > On 21 Dec 2014, at 22:43, Rickard Strandqvist > wrote: > > The NULL check was done to late, and there it was a risk > of a possible null pointer dereference. > > This was partially found by using a static code analysis program called > cppcheck. > > Signed-off-by: Rickard Strandqvist > --- > drivers/staging/lustre/lustre/include/lustre_update.h |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/lustre/lustre/include/lustre_update.h > b/drivers/staging/lustre/lustre/include/lustre_update.h > index 84defce..00e1361 100644 > --- a/drivers/staging/lustre/lustre/include/lustre_update.h > +++ b/drivers/staging/lustre/lustre/include/lustre_update.h > @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct > update_reply *reply, void **buf, > int result; > > ptr = update_get_buf_internal(reply, index, &size); > + > + LASSERT((ptr != NULL && size >= sizeof(int))); > + > result = *(int *)ptr; > > if (result < 0) > return result; > > - LASSERT((ptr != NULL && size >= sizeof(int))); This looks odd to me, LASSERT is essentially BUG_ON() so is used for checking logic bugs. Moving LASSERT calls doesn’t seem the correct way of resolving a logic problem and if you’re doing static analysis it might be more productive to do it this with LASSERT disabled. > *buf = ptr + sizeof(int); > return size - sizeof(int); > } > -- > 1.7.10.4 > > ___ > HPDD-discuss mailing list > hpdd-disc...@lists.01.org > https://lists.01.org/mailman/listinfo/hpdd-discuss ___ HPDD-discuss mailing list hpdd-disc...@lists.01.org https://lists.01.org/mailman/listinfo/hpdd-discuss -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 03/16] staging/lustre/nfs: writing to new files will return ENOENT
Peng,
I'm sorry to say, this patch was reverted due to interoperability problems with
2.1 servers (This either a slightly later [but still broken] version of the
patch we discussed recently, or the same one.). If Greg's accepted this
upstream (and it looks like he has), it'll have to be reverted.
Sorry for the trouble here!
- Patrick
From: Peng Tao [bergw...@gmail.com]
Sent: Monday, November 25, 2013 8:04 PM
To: Greg Kroah-Hartman
Cc: linux-kernel@vger.kernel.org; Patrick Farrell; Cheng Shao; Peng Tao;
Andreas Dilger
Subject: [PATCH 03/16] staging/lustre/nfs: writing to new files will return
ENOENT
From: Patrick Farrell
This happend with SLES11SP2 Lustre client, which in turn acts as an
NFS server, exporting a subtree of an Lustre fs through NFS.
We detected that whenever we are writing to a new file using, fx,
'echo blah > newfile', it will return ENOENT error. We found
out that this was caused by the anonymous dentry. In SLESS11SP2,
anonymous dentries are assigned '/' as the name, instead of an
empty string. When MDT handles the intent_open call, it will look
up the obj by the name if it is not an empty string, and thus
couldn't find it.
As MDS_OPEN_BY_FID is always set on this request, we never need
to send the name in this request. The fid is already available
and should be used in case the file has been renamed.
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-3544
Lustre-change: http://review.whamcloud.com/6920
Signed-off-by: Cheng Shao
Signed-off-by: Patrick Farrell
Reviewed-by: Bob Glossman
Reviewed-by: Alexey Shvetsov
Reviewed-by: Lai Siyao
Reviewed-by: James Simmons
Reviewed-by: Oleg Drokin
Signed-off-by: Peng Tao
Signed-off-by: Andreas Dilger
---
drivers/staging/lustre/lustre/llite/file.c |5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/lustre/lustre/llite/file.c
b/drivers/staging/lustre/lustre/llite/file.c
index 82248e9..f36c5d8 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -368,8 +368,6 @@ static int ll_intent_file_open(struct file *file, void *lmm,
{
struct ll_sb_info *sbi = ll_i2sbi(file->f_dentry->d_inode);
struct dentry *parent = file->f_dentry->d_parent;
- const char *name = file->f_dentry->d_name.name;
- const int len = file->f_dentry->d_name.len;
struct md_op_data *op_data;
struct ptlrpc_request *req;
__u32 opc = LUSTRE_OPC_ANY;
@@ -394,8 +392,9 @@ static int ll_intent_file_open(struct file *file, void *lmm,
}
op_data = ll_prep_md_op_data(NULL, parent->d_inode,
- file->f_dentry->d_inode, name, len,
+ file->f_dentry->d_inode, NULL, 0,
O_RDWR, opc, NULL);
+
if (IS_ERR(op_data))
return PTR_ERR(op_data);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [lustre-devel] [PATCH] staging: lustre: add __user attributes to llite/file.c
Greg just recently replied to a similar patch rejecting it. I don't have his response handy, but I bet you can find it in the archives. Briefly, this hides possible errors rather than fixing them. From: lustre-devel [lustre-devel-boun...@lists.lustre.org] on behalf of Wim de With [nauxu...@wimdewith.com] Sent: Tuesday, December 08, 2015 2:34 PM To: oleg.dro...@intel.com; andreas.dil...@intel.com; gre...@linuxfoundation.org Cc: de...@driverdev.osuosl.org; Wim de With; linux-kernel@vger.kernel.org; lustre-de...@lists.lustre.org Subject: [lustre-devel] [PATCH] staging: lustre: add __user attributes to llite/file.c This fixes the following sparse warnings: drivers/staging/lustre/lustre/llite/file.c:1310:38:expected void const [noderef] *from drivers/staging/lustre/lustre/llite/file.c:1310:38:got struct ll_recreate_obj * drivers/staging/lustre/lustre/llite/file.c:1328:35: warning: incorrect type in argument 2 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:1328:35:expected void const [noderef] *from drivers/staging/lustre/lustre/llite/file.c:1328:35:got struct lu_fid * drivers/staging/lustre/lustre/llite/file.c:1475:35: warning: incorrect type in argument 2 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:1475:35:expected void const [noderef] *from drivers/staging/lustre/lustre/llite/file.c:1475:35:got struct lov_user_md_v1 * drivers/staging/lustre/lustre/llite/file.c:1500:35: warning: incorrect type in argument 2 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:1500:35:expected void const [noderef] *from drivers/staging/lustre/lustre/llite/file.c:1500:35:got struct lov_user_md_v1 *lumv1p drivers/staging/lustre/lustre/llite/file.c:1505:44: warning: incorrect type in argument 2 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:1505:44:expected void const [noderef] *from drivers/staging/lustre/lustre/llite/file.c:1505:44:got struct lov_user_md_v3 *lumv3p drivers/staging/lustre/lustre/llite/file.c:1516:17: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:1516:17:expected void const volatile [noderef] * drivers/staging/lustre/lustre/llite/file.c:1516:17:got unsigned short * drivers/staging/lustre/lustre/llite/file.c:1829:27: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:1829:27:expected void [noderef] *to drivers/staging/lustre/lustre/llite/file.c:1829:27:got void * drivers/staging/lustre/lustre/llite/file.c:2214:24: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2214:24:expected void const volatile [noderef] * drivers/staging/lustre/lustre/llite/file.c:2214:24:got int * drivers/staging/lustre/lustre/llite/file.c:2221:21: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2221:21:expected void const volatile [noderef] * drivers/staging/lustre/lustre/llite/file.c:2221:21:got int * drivers/staging/lustre/lustre/llite/file.c:2245:43: warning: incorrect type in argument 2 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2245:43:expected void const [noderef] *from drivers/staging/lustre/lustre/llite/file.c:2245:43:got char * drivers/staging/lustre/lustre/llite/file.c:2275:24: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2275:24:expected void const volatile [noderef] * drivers/staging/lustre/lustre/llite/file.c:2275:24:got int * drivers/staging/lustre/lustre/llite/file.c:2292:35: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2292:35:expected void [noderef] *to drivers/staging/lustre/lustre/llite/file.c:2292:35:got void * drivers/staging/lustre/lustre/llite/file.c:2299:44: warning: incorrect type in argument 2 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2299:44:expected void [noderef] *arg drivers/staging/lustre/lustre/llite/file.c:2299:44:got void * drivers/staging/lustre/lustre/llite/file.c:2304:43: warning: incorrect type in argument 2 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2304:43:expected void const [noderef] *from drivers/staging/lustre/lustre/llite/file.c:2304:43:got char * drivers/staging/lustre/lustre/llite/file.c:2310:46: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2310:46:expected void [noderef] *to drivers/staging/lustre/lustre/llite/file.c:2310:46:got char * drivers/staging/lustre/lustre/llite/file.c:2323:21: warning: incorrect type in argument 1 (different address spaces) drivers/staging/lustre/lustre/llite/file.c:2323:21
RE: [HPDD-discuss] [PATCH] staging: lustre: fix sparse warnings related to lock context imbalance
Dan,
I disagree about the change suggested here. In this particular code,
'object_attr' is distinct from 'attr', as in a 'setattr' call on an inode.
'cl_object' is a distinct thing from an inode/file on disk, and specifying it
is the objects attr is helpful in understanding there is not a direct
relationship to 'attr' in the general filesystem sense. (cl_object attrs are
used in determining actual on disk attributes, but there is not a one-to-one
correspondence.)
I am willing to be corrected, but that is my first feeling here.
- Patrick
From: HPDD-discuss [hpdd-discuss-boun...@lists.01.org] on behalf of Dan
Carpenter [dan.carpen...@oracle.com]
Sent: Friday, November 28, 2014 4:00 AM
To: Loïc Pefferkorn
Cc: de...@driverdev.osuosl.org; Greg KH; linux-kernel@vger.kernel.org;
gdon...@gmail.com; hpdd-disc...@ml01.01.org
Subject: Re: [HPDD-discuss] [PATCH] staging: lustre: fix sparse warnings
related to lock context imbalance
On Thu, Nov 27, 2014 at 07:34:10PM +0100, Loïc Pefferkorn wrote:
> 1827 if (valid != 0) {
> 1828 cl_object_attr_lock(obj);
> 1829 cl_object_attr_set(env, obj, attr, valid);
> 1830 cl_object_attr_unlock(obj);
>
> after:
>
> 1827 if (valid != 0) {
> 1828 spin_lock(cl_object_attr_guard(obj));
> 1829 cl_object_attr_set(env, obj, attr, valid);
> 1830 spin_unlock(cl_object_attr_guard(obj));
The word "_object" doesn't add any new information to the name. If you
remove it then the code is improved.
spin_lock(cl_attr_guard(obj));
cl_attr_set(env, obj, attr, valid);
spin_unlock(cl_attr_guard(obj));
regards,
dan carpenter
___
HPDD-discuss mailing list
hpdd-disc...@lists.01.org
https://lists.01.org/mailman/listinfo/hpdd-discuss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [HPDD-discuss] [PATCH v4 10/13] staging: lustre: lnet: lnet: checkpatch.pl fixes
Since it is not actually doing a printk - at least, not necessarily - I like lustre_logmsg. lustre_output seems too vague. - Patrick From: HPDD-discuss [hpdd-discuss-boun...@lists.01.org] on behalf of Joe Perches [j...@perches.com] Sent: Friday, May 22, 2015 7:36 PM To: Drokin, Oleg Cc: ; ; ; ; Julia Lawall; ; Subject: Re: [HPDD-discuss] [PATCH v4 10/13] staging: lustre: lnet: lnet: checkpatch.pl fixes On Sat, 2015-05-23 at 00:25 +, Drokin, Oleg wrote: > On May 22, 2015, at 8:18 PM, Joe Perches wrote: > I wonder what is more clear about that in your opinion ve > lustre_error/lustre_debug? > >>> > >>> The fact that you have to explain this shows that it's > >>> at least misleading unless you completely understand the > >>> code. > >> > >> Or you know, you might take the function name at the face value > >> and assume that CERROR means it's an error and CDEBUG means it's a debug > >> message? > > > > Maybe, but I think that it'd be better if the mechanism > > it uses was more plainly named something like lustre_log. > > While the idea seems good, the biggest obstacle here is such that > there's already a thing called lustre log (llog for short too) - > it's kind of a distributed journal of operations. > > Its there a different synonym, I wonder? Maybe: lustre_printk, lustre_logmsg, lustre_output ___ HPDD-discuss mailing list hpdd-disc...@lists.01.org https://lists.01.org/mailman/listinfo/hpdd-discuss -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [HPDD-discuss] [PATCH] Staging: lustre: file.c: fix coding style
Uck, my reply made the formatting even worse. I'm trying to say it
should look like this:
+rc = ll_intent_file_open(file->f_path.dentry,
+NULL, 0, it);
Not like this:
+rc = ll_intent_file_open(file->f_path.dentry,
+ NULL, 0, it);
On 03/18/2015 02:31 PM, Patrick Farrell wrote:
Perhaps this is just a formatting error in my email client, but
shouldn't NULL be one more space over to line up with the '(' above?
On 03/18/2015 02:08 PM, p...@amd48-systeme.lip6.fr wrote:
+rc = ll_intent_file_open(file->f_path.dentry,
+NULL, 0, it);
___
HPDD-discuss mailing list
hpdd-disc...@lists.01.org
https://lists.01.org/mailman/listinfo/hpdd-discuss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [HPDD-discuss] [PATCH] Staging: lustre: file.c: fix coding style
Perhaps this is just a formatting error in my email client, but
shouldn't NULL be one more space over to line up with the '(' above?
On 03/18/2015 02:08 PM, p...@amd48-systeme.lip6.fr wrote:
+ rc = ll_intent_file_open(file->f_path.dentry,
+ NULL, 0, it);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [HPDD-discuss] [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference
Strongly agreed that execution speed is not critical here. It's the update of
a proc variable, not a tight loop or critical section.
Normally I'd leave it alone, but since you're writing a patch anyway, I'd do
'tolower' myself. I dislike the stacked case statements on a single line like
that. (It's the only time I've seen them written that way. Perhaps it's
common and I've just missed it.)
Regards,
- Patrick
From: HPDD-discuss [hpdd-discuss-boun...@lists.01.org] on behalf of Joe Perches
[j...@perches.com]
Sent: Monday, December 15, 2014 5:53 PM
To: Rickard Strandqvist
Cc: de...@driverdev.osuosl.org; Fabian Frederick; Julia Lawall; James Simmons;
Greg Kroah-Hartman; linux-kernel@vger.kernel.org; Greg Donald;
hpdd-disc...@lists.01.org; Andriy Skulysh
Subject: Re: [HPDD-discuss] [PATCH] staging: lustre: lustre: obdclass:
lprocfs_status.c: Fix for possible null pointer dereference
On Mon, 2014-12-15 at 23:23 +0100, Rickard Strandqvist wrote:
> Hi Joe
Hello Rickard
> No, it does not look like end can be NULL then.
> Then remove the end != NULL instead?
> ...
> if (end != NULL && *end == '.') {
Up to you.
> However, I am hesitant to the tolower() I think double case is faster...?
I doubt code execution speed is paramount here.
Maybe see if the object code size is smaller one
way or the other.
___
HPDD-discuss mailing list
hpdd-disc...@lists.01.org
https://lists.01.org/mailman/listinfo/hpdd-discuss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [lustre-devel] [PATCH 13/30] staging: lustre: replace libcfs_register_ioctl with a blocking notifier_chain
This, and the rest of the series, look good. Feel free to add a Reviewed-by.
Thanks, Neil.
On 5/20/18, 11:39 PM, "lustre-devel on behalf of NeilBrown"
wrote:
libcfs allows other modules to register handlers for ioctls.
The implementation it uses for this is nearly identical to a
blocking notifier chain, so change to use that.
The biggest difference is that the return value from notifier has a
defined format, where libcfs_register_ioctl uses -EINVAL to mean
"continue". This requires a little bit of conversion.
Signed-off-by: NeilBrown
---
.../staging/lustre/include/linux/libcfs/libcfs.h | 19 ++
drivers/staging/lustre/lnet/libcfs/module.c| 64
drivers/staging/lustre/lnet/lnet/module.c | 38
drivers/staging/lustre/lnet/selftest/conctl.c | 27 +---
drivers/staging/lustre/lnet/selftest/console.c | 10 ++-
drivers/staging/lustre/lnet/selftest/console.h |3 +
6 files changed, 70 insertions(+), 91 deletions(-)
diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs.h
b/drivers/staging/lustre/include/linux/libcfs/libcfs.h
index d9002e7424d4..63ea0e99ec58 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs.h
@@ -93,19 +93,14 @@
#define LNET_ACCEPTOR_MIN_RESERVED_PORT512
#define LNET_ACCEPTOR_MAX_RESERVED_PORT1023
-struct libcfs_ioctl_handler {
- struct list_head item;
- int (*handle_ioctl)(unsigned int cmd, struct libcfs_ioctl_hdr *hdr);
-};
-
-#define DECLARE_IOCTL_HANDLER(ident, func) \
- struct libcfs_ioctl_handler ident = { \
- .item = LIST_HEAD_INIT(ident.item), \
- .handle_ioctl = func \
- }
+extern struct blocking_notifier_head libcfs_ioctl_list;
+static inline int notifier_from_ioctl_errno(int err)
+{
+ if (err == -EINVAL)
+ return NOTIFY_OK;
+ return notifier_from_errno(err) | NOTIFY_STOP_MASK;
+}
-int libcfs_register_ioctl(struct libcfs_ioctl_handler *hand);
-int libcfs_deregister_ioctl(struct libcfs_ioctl_handler *hand);
int libcfs_setup(void);
#define _LIBCFS_H
diff --git a/drivers/staging/lustre/lnet/libcfs/module.c
b/drivers/staging/lustre/lnet/libcfs/module.c
index 3e51aae751c5..b3a7c1a912ba 100644
--- a/drivers/staging/lustre/lnet/libcfs/module.c
+++ b/drivers/staging/lustre/lnet/libcfs/module.c
@@ -62,38 +62,8 @@
static struct dentry *lnet_debugfs_root;
-static DECLARE_RWSEM(ioctl_list_sem);
-static LIST_HEAD(ioctl_list);
-
-int libcfs_register_ioctl(struct libcfs_ioctl_handler *hand)
-{
- int rc = 0;
-
- down_write(&ioctl_list_sem);
- if (!list_empty(&hand->item))
- rc = -EBUSY;
- else
- list_add_tail(&hand->item, &ioctl_list);
- up_write(&ioctl_list_sem);
-
- return rc;
-}
-EXPORT_SYMBOL(libcfs_register_ioctl);
-
-int libcfs_deregister_ioctl(struct libcfs_ioctl_handler *hand)
-{
- int rc = 0;
-
- down_write(&ioctl_list_sem);
- if (list_empty(&hand->item))
- rc = -ENOENT;
- else
- list_del_init(&hand->item);
- up_write(&ioctl_list_sem);
-
- return rc;
-}
-EXPORT_SYMBOL(libcfs_deregister_ioctl);
+BLOCKING_NOTIFIER_HEAD(libcfs_ioctl_list);
+EXPORT_SYMBOL(libcfs_ioctl_list);
static inline size_t libcfs_ioctl_packlen(struct libcfs_ioctl_data *data)
{
@@ -268,24 +238,18 @@ static int libcfs_ioctl(unsigned long cmd, void
__user *uparam)
libcfs_debug_mark_buffer(data->ioc_inlbuf1);
break;
- default: {
- struct libcfs_ioctl_handler *hand;
-
- err = -EINVAL;
- down_read(&ioctl_list_sem);
- list_for_each_entry(hand, &ioctl_list, item) {
- err = hand->handle_ioctl(cmd, hdr);
- if (err == -EINVAL)
- continue;
-
- if (!err) {
- if (copy_to_user(uparam, hdr, hdr->ioc_len))
- err = -EFAULT;
- }
- break;
- }
- up_read(&ioctl_list_sem);
- break; }
+ default:
+ err = blocking_notifier_call_chain(&libcfs_ioctl_list,
+ cmd, hdr);
+ if (!(err & NOTIFY_STOP_MASK))
+ /* No-one claimed the ioctl */
+ err = -EINVAL;
+ else
+ err = notifier_to_errno(err);
Re: [lustre-devel] [PATCH 1/6] staging: lustre: move stack-check macros to libcfs_debug.h
James,
If I understand correctly, you're saying you want to be able to build without
debug support...? I'm not convinced that building a client without debug
support is interesting or useful. In fact, I think it would be harmful, and we
shouldn't open up the possibility - this is switchable debug with very low
overhead when not actually "on". It would be really awful to get a problem on
a running system and discover there's no debug support - that you can't even
enable debug without a reinstall.
If I've understood you correctly, then I would want to see proof of a
significant performance cost when debug is built but *off* before agreeing to
even exposing this option. (I know it's a choice they'd have to make, but if
it's not really useful with a side order of potentially harmful, we shouldn't
even give people the choice.)
- Patrick
On 4/15/18, 10:49 PM, "lustre-devel on behalf of James Simmons"
wrote:
> CDEBUG_STACK() and CHECK_STACK() are macros to help with
> debugging, so move them from
>drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
> to
>drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
>
> This seems a more fitting location, and is a step towards
> removing linux/libcfs.h and simplifying the include file structure.
Nak. Currently the lustre client always enables debugging but that
shouldn't be the case. What we do need is the able to turn off the
crazy debugging stuff. In the development branch of lustre it is
done with CDEBUG_ENABLED. We need something like that in Kconfig
much like we have CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK. Since we like
to be able to turn that off this should be moved to just after
LIBCFS_DEBUG_MSG_DATA_DECL. Then from CHECK_STACK down to CWARN()
it can be build out. When CDEBUG_ENABLED is disabled CDEBUG_LIMIT
would be empty.
> Signed-off-by: NeilBrown
> ---
> .../lustre/include/linux/libcfs/libcfs_debug.h | 32
> .../lustre/include/linux/libcfs/linux/libcfs.h | 31
---
> 2 files changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
b/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
> index 9290a19429e7..0dc7b91efe7c 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
> @@ -62,6 +62,38 @@ int libcfs_debug_str2mask(int *mask, const char *str,
int is_subsys);
> extern unsigned int libcfs_catastrophe;
> extern unsigned int libcfs_panic_on_lbug;
>
> +/* Enable debug-checks on stack size - except on x86_64 */
> +#if !defined(__x86_64__)
> +# ifdef __ia64__
> +# define CDEBUG_STACK() (THREAD_SIZE - \
> + ((unsigned long)__builtin_dwarf_cfa() & \
> +(THREAD_SIZE - 1)))
> +# else
> +# define CDEBUG_STACK() (THREAD_SIZE - \
> + ((unsigned long)__builtin_frame_address(0) & \
> +(THREAD_SIZE - 1)))
> +# endif /* __ia64__ */
> +
> +#define __CHECK_STACK(msgdata, mask, cdls) \
> +do { \
> + if (unlikely(CDEBUG_STACK() > libcfs_stack)) {\
> + LIBCFS_DEBUG_MSG_DATA_INIT(msgdata, D_WARNING, NULL); \
> + libcfs_stack = CDEBUG_STACK();\
> + libcfs_debug_msg(msgdata, \
> + "maximum lustre stack %lu\n",\
> + CDEBUG_STACK()); \
> + (msgdata)->msg_mask = mask; \
> + (msgdata)->msg_cdls = cdls; \
> + dump_stack(); \
> + /*panic("LBUG");*/\
> + } \
> +} while (0)
> +#define CFS_CHECK_STACK(msgdata, mask, cdls) __CHECK_STACK(msgdata,
mask, cdls)
> +#else /* __x86_64__ */
> +#define CFS_CHECK_STACK(msgdata, mask, cdls) do {} while (0)
> +#define CDEBUG_STACK() (0L)
> +#endif /* __x86_64__ */
> +
> #ifndef DEBUG_SUBSYSTEM
> # define DEBUG_SUBSYSTEM S_UNDEFINED
> #endif
> diff --git a/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
b/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
> index 07d3cb2217d1..83aec9c7698f 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
> @@ -80,35 +80,4 @@
> #include
> #include "linux-
Re: [lustre-devel] [PATCH 00/21] staging: assorted lustre clean-up
Lustre networking driver. (Or so I was taught)
Regards,
Patrick
From: lustre-devel on behalf of
NeilBrown
Sent: Monday, February 19, 2018 8:23:37 PM
To: Oleg Drokin; James Simmons; Andreas Dilger; Greg Kroah-Hartman
Cc: lkml; lustre
Subject: [lustre-devel] [PATCH 00/21] staging: assorted lustre clean-up
Following are assorted patches which clean up parts of lustre.
One or two tiny buglets are fixed, but mostly the patches
just make the code more readable, often by removing distractions.
The first several patches remove content from libcfs, completely
removing 3 files.
There are some list_for_each improvements in ptlrpc, and some
code simplification in fid.
Finally a small simplification in socklnd.
(lnd ?? Lustre Networking Domain??)
Thanks,
NeilBrown
---
NeilBrown (21):
staging: lustre: replace all CFS_CAP_* macros with CAP_*
staging: lustre: opencode cfs_cap_{raise,lower,raised}
staging: lustre: remove linux-curproc.c
staging: lustre: remove unnecessary cfs_block_allsigs() calls
staging: lustre: lnet: remove cfs_block_allsigs calls.
staging: lustre: simplify linux-prim.c
staging: lustre: improve API and implementation of blocking signals.
staging: lustre: make signal-blocking functions inline
staging: lustre: discard libcfs_kvzalloc_cpt()
staging: lustre: discard lu_buf allocation library.
staging: lustre: improve some libcfs_kvzalloc calls.
staging: lustre: discard libcfs_kvzalloc and linux-mem.c
staging: lustre: remove phantom struct cfs_crypto_hash_desc
staging: lustre: fix assorted checkpatch errors
staging: lustre: ptlrpc: list_for_each improvements.
staging: lustre: fid: convert lcs_mutex to a spinlock
staging: lustre: fid: use wait_event_cmd()
staging: lustre: fid: remove seq_fid_alloc_fini() and simplify
staging: lustre: fid: fix up debugfs access to ->lcs_space
staging: lustre: fid: perform sanity checks before commiting
staging: lustre: socklnd: simplify ksnc_rx_iov_space
.../staging/lustre/include/linux/libcfs/curproc.h | 37 ++-
.../staging/lustre/include/linux/libcfs/libcfs.h | 27 +++--
.../lustre/include/linux/libcfs/libcfs_crypto.h| 11 +-
drivers/staging/lustre/include/linux/lnet/api.h|1
.../staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c |6 -
.../staging/lustre/lnet/klnds/socklnd/socklnd.h| 11 --
.../staging/lustre/lnet/klnds/socklnd/socklnd_cb.c | 10 --
.../lustre/lnet/klnds/socklnd/socklnd_lib.c|2
drivers/staging/lustre/lnet/libcfs/Makefile|4 -
.../staging/lustre/lnet/libcfs/linux/linux-cpu.c |2
.../lustre/lnet/libcfs/linux/linux-crypto.c| 29 ++---
.../lustre/lnet/libcfs/linux/linux-curproc.c | 108 ---
.../staging/lustre/lnet/libcfs/linux/linux-mem.c | 51 -
.../staging/lustre/lnet/libcfs/linux/linux-prim.c | 113
drivers/staging/lustre/lnet/lnet/acceptor.c|2
drivers/staging/lustre/lnet/lnet/api-ni.c | 15 +--
drivers/staging/lustre/lnet/lnet/lib-eq.c | 10 +-
drivers/staging/lustre/lnet/lnet/router.c |2
drivers/staging/lustre/lnet/selftest/timer.c |2
drivers/staging/lustre/lustre/fid/fid_request.c| 106 ++-
drivers/staging/lustre/lustre/fid/lproc_fid.c | 44 +---
drivers/staging/lustre/lustre/include/lu_object.h |7 -
drivers/staging/lustre/lustre/include/lustre_fid.h |2
drivers/staging/lustre/lustre/include/lustre_lib.h | 18 ++-
drivers/staging/lustre/lustre/include/lustre_sec.h |3 -
drivers/staging/lustre/lustre/include/obd_class.h |8 +
.../staging/lustre/lustre/include/obd_support.h|2
drivers/staging/lustre/lustre/llite/dcache.c |2
drivers/staging/lustre/lustre/llite/dir.c | 10 +-
drivers/staging/lustre/lustre/llite/file.c | 12 +-
drivers/staging/lustre/lustre/llite/llite_lib.c|8 +
drivers/staging/lustre/lustre/llite/llite_mmap.c |8 +
drivers/staging/lustre/lustre/llite/xattr.c|2
drivers/staging/lustre/lustre/lmv/lmv_obd.c|4 -
drivers/staging/lustre/lustre/lov/lov_ea.c |2
drivers/staging/lustre/lustre/lov/lov_io.c |2
drivers/staging/lustre/lustre/lov/lov_lock.c |2
drivers/staging/lustre/lustre/lov/lov_object.c |4 -
drivers/staging/lustre/lustre/lov/lov_pack.c |2
drivers/staging/lustre/lustre/lov/lov_request.c|2
drivers/staging/lustre/lustre/mdc/mdc_locks.c |2
drivers/staging/lustre/lustre/mdc/mdc_request.c|6 +
drivers/staging/lustre/lustre/obdclass/linkea.c| 16 ++-
.../lustre/lustre/obdclass/linux/linux-module.c|4 -
drivers/staging/lustre/lustre/obdclass/llog.c | 22 ++--
drivers/staging/lustre/lustre/obdclass/lu_object.c | 70
.../lustre/lu
Re: [lustre-devel] [PATCH 06/19] staging: lustre: introduce and use l_wait_event_abortable()
It's worth noting that the change from -EINTR to -ERESTARTSYS will modify the
behavior of userspace slightly. Specifically, when a signal handler is setup
with retry set (SA_RESTART flag set), the syscall will be restarted rather than
aborted.
This should be fine. It may eventually shake out some stuble bugs in Lustre
when we abort an operation and then restart it from a point where we didn't in
the past - past instances where we changed from -EINTR to -ERESTARTSYS
certainly have - but it's for the best. As I understand it from past
conversations with Andreas and others, Lustre is not really intending to claim
it's not restartable, it's just an artifact of people copying older code that
was written without awareness of the difference in EINTR vs ERESTARTSYS.
Ideally someone should go through and audit the remaining uses of -EINTR and
replace most of them with -ERESTARTSYS.
James, maybe you want to add that to the TODO list?
On 2/12/18, 3:24 PM, "lustre-devel on behalf of NeilBrown"
wrote:
lustre sometimes wants to wait for an event, but abort if
one of a specific list of signals arrives. This is a little
bit like wait_event_killable(), except that the signals are
identified a different way.
So introduce l_wait_event_abortable() which provides this
functionality.
Having separate functions for separate needs is more in line
with the pattern set by include/linux/wait.h, than having a
single function which tries to include all possible needs.
Also introduce l_wait_event_abortable_exclusive().
Note that l_wait_event() return -EINTR on a signal, while
Linux wait_event functions return -ERESTARTSYS.
l_wait_event_{abortable_,}exclusive follow the Linux pattern.
Reviewed-by: James Simmons
Signed-off-by: NeilBrown
---
drivers/staging/lustre/lustre/include/lustre_lib.h | 24
drivers/staging/lustre/lustre/ldlm/ldlm_resource.c | 12 +-
drivers/staging/lustre/lustre/llite/llite_lib.c| 12 +++---
drivers/staging/lustre/lustre/obdclass/genops.c|9 +++-
drivers/staging/lustre/lustre/obdclass/llog_obd.c |5 ++--
drivers/staging/lustre/lustre/osc/osc_page.c |6 ++---
drivers/staging/lustre/lustre/osc/osc_request.c|6 ++---
7 files changed, 43 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h
b/drivers/staging/lustre/lustre/include/lustre_lib.h
index 7d950c53e962..b2a64d0e682c 100644
--- a/drivers/staging/lustre/lustre/include/lustre_lib.h
+++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
@@ -336,4 +336,28 @@ do {
\
/** @} lib */
+
+/* l_wait_event_abortable() is a bit like wait_event_killable()
+ * except there is a fixed set of signals which will abort:
+ * LUSTRE_FATAL_SIGS
+ */
+#define l_wait_event_abortable(wq, condition)
\
+({ \
+ sigset_t __blocked; \
+ int __ret = 0; \
+ __blocked = cfs_block_sigsinv(LUSTRE_FATAL_SIGS); \
+ __ret = wait_event_interruptible(wq, condition);\
+ cfs_restore_sigs(__blocked);\
+ __ret; \
+})
+
+#define l_wait_event_abortable_exclusive(wq, condition)
\
+({ \
+ sigset_t __blocked; \
+ int __ret = 0; \
+ __blocked = cfs_block_sigsinv(LUSTRE_FATAL_SIGS); \
+ __ret = wait_event_interruptible_exclusive(wq, condition); \
+ cfs_restore_sigs(__blocked);\
+ __ret; \
+})
#endif /* _LUSTRE_LIB_H */
diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
b/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
index 2e66825c8f4b..4c44603ab6f9 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c
@@ -879,7 +879,6 @@ static int __ldlm_namespace_free(struct ldlm_namespace
*ns, int force)
ldlm_namespace_cleanup(ns, force ? LDLM_FL_LOCAL_ONLY : 0);
if (atomic_read(&ns->ns_bref) > 0) {
- struct l_wait_info lwi = LWI_INTR(LWI_ON_SIGNAL_NOOP, NULL);
int rc;
CDEBUG(D_DLMTRACE,
@@ -887,11 +886,12 @@ static int __ldlm_namespace_free(struct
ldlm_namespace *n
Re: [lustre-devel] [PATCH 08/19] staging: lustre: simplify waiting in ldlm_completion_ast()
Neil,
I didn't get anything after 8/19 in this series. Is this just me? (I'd keep
waiting, except I also found a few things in this patch.)
Minor:
The line XXX ALLOCATE is out of date and could go. (It refers to a mix of
things you eliminated and things that were already gone.)
Less minor:
You remove use of the imp_lock when reading the connection count. While
that'll work on x86, it's probably wrong on some architecture to read that
without taking the lock...?
Bug:
The existing code uses the imp_conn_cnt from *before* the wait, rather than
after. I think that's quite important. So you'll want to read it out before
the wait. I think the main reason we'd hit the timeout is a disconnect, which
should cause a reconnect, so it's very important to use the value from *before*
the wait. (See comment on ptlrpc_set_import_discon for more of an explanation.
Basically it's tracking a connection 'epoch', if it's changed, someone else
already went through the reconnect code for this 'connection epoch' and we
shouldn't start that process.)
- Patrick
On 2/12/18, 3:24 PM, "lustre-devel on behalf of NeilBrown"
wrote:
If a signal-callback (lwi_on_signal) is set without lwi_allow_intr, as
is the case in ldlm_completion_ast(), the behavior depends on the
timeout set.
If a timeout is set, then signals are ignored. If the timeout is
reached, the timeout handler is called. If the timeout handler
return 0, which ldlm_expired_completion_wait() always does, the
l_wait_event() switches to exactly the behavior if no timeout was set.
If no timeout is set, then "fatal" signals are not ignored. If one
arrives the callback is run, but as the callback is empty in this
case, that is not relevant.
This can be simplified to:
if a timeout is wanted
wait_event_idle_timeout()
if that timed out, call the timeout handler
l_wait_event_abortable()
i.e. the code always waits indefinitely. Sometimes it performs a
non-abortable wait first. Sometimes it doesn't. But it only
aborts before the condition is true if it is signaled.
This doesn't quite agree with the comments and debug messages.
Reviewed-by: James Simmons
Signed-off-by: NeilBrown
---
drivers/staging/lustre/lustre/ldlm/ldlm_request.c | 55
+++--
1 file changed, 18 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_request.c
b/drivers/staging/lustre/lustre/ldlm/ldlm_request.c
index a244fa717134..f1233d844bbd 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_request.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_request.c
@@ -72,15 +72,6 @@ MODULE_PARM_DESC(ldlm_enqueue_min, "lock enqueue timeout
minimum");
/* in client side, whether the cached locks will be canceled before replay
*/
unsigned int ldlm_cancel_unused_locks_before_replay = 1;
-static void interrupted_completion_wait(void *data)
-{
-}
-
-struct lock_wait_data {
- struct ldlm_lock *lwd_lock;
- __u32lwd_conn_cnt;
-};
-
struct ldlm_async_args {
struct lustre_handle lock_handle;
};
@@ -112,10 +103,8 @@ static int ldlm_request_bufsize(int count, int type)
return sizeof(struct ldlm_request) + avail;
}
-static int ldlm_expired_completion_wait(void *data)
+static void ldlm_expired_completion_wait(struct ldlm_lock *lock, struct
obd_import *imp2)
{
- struct lock_wait_data *lwd = data;
- struct ldlm_lock *lock = lwd->lwd_lock;
struct obd_import *imp;
struct obd_device *obd;
@@ -135,19 +124,17 @@ static int ldlm_expired_completion_wait(void *data)
if (last_dump == 0)
libcfs_debug_dumplog();
}
- return 0;
+ return;
}
obd = lock->l_conn_export->exp_obd;
imp = obd->u.cli.cl_import;
- ptlrpc_fail_import(imp, lwd->lwd_conn_cnt);
+ ptlrpc_fail_import(imp, imp2 ? imp2->imp_conn_cnt : 0);
LDLM_ERROR(lock,
"lock timed out (enqueued at %lld, %llds ago), entering
recovery for %s@%s",
(s64)lock->l_last_activity,
(s64)(ktime_get_real_seconds() - lock->l_last_activity),
obd2cli_tgt(obd), imp->imp_connection->c_remote_uuid.uuid);
-
- return 0;
}
/**
@@ -251,10 +238,8 @@ EXPORT_SYMBOL(ldlm_completion_ast_async);
int ldlm_completion_ast(struct ldlm_lock *lock, __u64 flags, void *data)
{
/* XXX ALLOCATE - 160 bytes */
- struct lock_wait_data lwd;
struct obd_device *obd;
struct obd_import *imp = NULL;
- struct l_wait_info lwi;
__u32 timeout;
int rc = 0;
@@ -281,32 +266,28 @@ int ldlm_completion_ast(struct ldlm_lock *lock, __u64
flags
Re: [lustre-devel] [PATCH 19/19] staging: lustre: remove l_wait_event() and related code
With the fix from yesterday, these look good. Thanks, Neil.
Reviewed-by: Patrick Farrell
- Patrick
On 2/12/18, 5:52 PM, "lustre-devel on behalf of NeilBrown"
wrote:
These macros are no longer used, so they can
be removed.
Reviewed-by: James Simmons
Signed-off-by: NeilBrown
---
drivers/staging/lustre/lustre/include/lustre_lib.h | 249
1 file changed, 249 deletions(-)
diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h
b/drivers/staging/lustre/lustre/include/lustre_lib.h
index ccc1a329e42b..1efd86f18c1f 100644
--- a/drivers/staging/lustre/lustre/include/lustre_lib.h
+++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
@@ -76,123 +76,6 @@ int do_set_info_async(struct obd_import *imp,
void target_send_reply(struct ptlrpc_request *req, int rc, int fail_id);
-/*
- * l_wait_event is a flexible sleeping function, permitting simple caller
- * configuration of interrupt and timeout sensitivity along with actions to
- * be performed in the event of either exception.
- *
- * The first form of usage looks like this:
- *
- * struct l_wait_info lwi = LWI_TIMEOUT_INTR(timeout, timeout_handler,
- *intr_handler, callback_data);
- * rc = l_wait_event(waitq, condition, &lwi);
- *
- * l_wait_event() makes the current process wait on 'waitq' until
'condition'
- * is TRUE or a "killable" signal (SIGTERM, SIKGILL, SIGINT) is pending.
It
- * returns 0 to signify 'condition' is TRUE, but if a signal wakes it
before
- * 'condition' becomes true, it optionally calls the specified
'intr_handler'
- * if not NULL, and returns -EINTR.
- *
- * If a non-zero timeout is specified, signals are ignored until the
timeout
- * has expired. At this time, if 'timeout_handler' is not NULL it is
called.
- * If it returns FALSE l_wait_event() continues to wait as described above
with
- * signals enabled. Otherwise it returns -ETIMEDOUT.
- *
- * LWI_INTR(intr_handler, callback_data) is shorthand for
- * LWI_TIMEOUT_INTR(0, NULL, intr_handler, callback_data)
- *
- * The second form of usage looks like this:
- *
- * struct l_wait_info lwi = LWI_TIMEOUT(timeout, timeout_handler);
- * rc = l_wait_event(waitq, condition, &lwi);
- *
- * This form is the same as the first except that it COMPLETELY IGNORES
- * SIGNALS. The caller must therefore beware that if 'timeout' is zero,
or if
- * 'timeout_handler' is not NULL and returns FALSE, then the ONLY thing
that
- * can unblock the current process is 'condition' becoming TRUE.
- *
- * Another form of usage is:
- * struct l_wait_info lwi = LWI_TIMEOUT_INTERVAL(timeout, interval,
- *timeout_handler);
- * rc = l_wait_event(waitq, condition, &lwi);
- * This is the same as previous case, but condition is checked once every
- * 'interval' jiffies (if non-zero).
- *
- * Subtle synchronization point: this macro does *not* necessary takes
- * wait-queue spin-lock before returning, and, hence, following idiom is
safe
- * ONLY when caller provides some external locking:
- *
- * Thread1Thread2
- *
- * l_wait_event(&obj->wq, );(1)
- *
- * wake_up(&obj->wq): (2)
- * spin_lock(&q->lock); (2.1)
- * __wake_up_common(q, ...); (2.2)
- * spin_unlock(&q->lock, flags); (2.3)
- *
- * kfree(obj); (3)
- *
- * As l_wait_event() may "short-cut" execution and return without taking
- * wait-queue spin-lock, some additional synchronization is necessary to
- * guarantee that step (3) can begin only after (2.3) finishes.
- *
- * XXX nikita: some ptlrpc daemon threads have races of that sort.
- *
- */
-
-#define LWI_ON_SIGNAL_NOOP ((void (*)(void *))(-1))
-
-struct l_wait_info {
- long lwi_timeout;
- long lwi_interval;
- int lwi_allow_intr;
- int (*lwi_on_timeout)(void *);
- void (*lwi_on_signal)(void *);
- void *lwi_cb_data;
-};
-
-/* NB: LWI_TIMEOUT ignores signals completely */
-#define LWI_TIMEOUT(time, cb, data) \
-((struct l_wait_info) { \
- .lwi_timeout= time, \
- .lwi_on_timeout = cb, \
- .lwi_cb_data= data, \
- .lwi_interval =
Re: [lustre-devel] [PATCH 02/16] staging: lustre: replace simple cases of l_wait_event() with wait_event().
Ah, finally we¹ve got that NOLOAD flag! This will clear up several nasty
bugs around ptrace and sigkill that come when waiting with signals blocked
in TASK_INTERRUPTIBLE. I see it was added in 2015Š The joys of working
with vendor kernels.
Thanks for these, Neil. I¹ll try to take a careful look.
Given the description of the commit that added TASK_NOLOAD, I¹m kind of
shocked to see it has almost no users in the kernel yet. Just a few
callers of schedule_timeout_idle, it looks like. Even so, why not put
those macros in sched.h to begin with?
- Patrick
On 12/18/17, 1:17 AM, "lustre-devel on behalf of NeilBrown"
wrote:
>When the lwi arg is full of zeros, l_wait_event() behaves almost
>identically to the standard wait_event() interface, so use that
>instead.
>
>The only difference in behavior is that l_wait_event() blocks all
>signals and uses an TASK_INTERRUPTIBLE wait, while wait_event()
>does not block signals, but waits in state TASK_UNINTERRUPTIBLE.
>This means that processes blocked in wait_event() will contribute
>to the load average. This behavior is (arguably) more correct - in
>most cases.
>
>In some cases, the wait is in a service thread waiting for work to
>do. In these case we should wait TASK_NOLOAD order with
>TASK_UNINTERRUPTIBLE. To facilitate this, add a "wait_event_noload()"
>macro. This should eventually be moved into include/linux/wait.h.
>
>There is one case where wait_event_exclusive_noload() is needed.
>So we add a macro for that too.
>
>Signed-off-by: NeilBrown
>---
> drivers/staging/lustre/lustre/include/lustre_lib.h | 47
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_lock.c |4 --
> drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c|8 +--
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c |5 +-
> drivers/staging/lustre/lustre/llite/statahead.c| 50
>
> drivers/staging/lustre/lustre/lov/lov_object.c |6 +-
> drivers/staging/lustre/lustre/mgc/mgc_request.c|4 --
> drivers/staging/lustre/lustre/obdclass/cl_io.c |6 +-
> drivers/staging/lustre/lustre/obdclass/genops.c| 15 ++
> drivers/staging/lustre/lustre/osc/osc_cache.c |5 +-
> drivers/staging/lustre/lustre/osc/osc_object.c |4 --
> drivers/staging/lustre/lustre/ptlrpc/pinger.c | 10 ++--
> drivers/staging/lustre/lustre/ptlrpc/sec_gc.c | 11 ++--
> drivers/staging/lustre/lustre/ptlrpc/service.c | 13 ++---
> 14 files changed, 93 insertions(+), 95 deletions(-)
>
>diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h
>b/drivers/staging/lustre/lustre/include/lustre_lib.h
>index ca1dce15337e..08bdd618ea7d 100644
>--- a/drivers/staging/lustre/lustre/include/lustre_lib.h
>+++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
>@@ -333,12 +333,6 @@ do {
> \
> __ret;\
> })
>
>-#define l_wait_condition(wq, condition)\
>-({ \
>- struct l_wait_info lwi = { 0 }; \
>- l_wait_event(wq, condition, &lwi);\
>-})
>-
> #define l_wait_condition_exclusive(wq, condition)\
> ({ \
> struct l_wait_info lwi = { 0 }; \
>@@ -353,4 +347,45 @@ do {
> \
>
> /** @} lib */
>
>+#define __wait_event_noload(wq_head, condition)
>\
>+ (void)___wait_event(wq_head, condition, (TASK_UNINTERRUPTIBLE |
>TASK_NOLOAD), 0, 0, \
>+ schedule())
>+
>+/**
>+ * wait_event_noload - sleep, without registering load, until a
>condition gets true
>+ * @wq_head: the waitqueue to wait on
>+ * @condition: a C expression for the event to wait for
>+ *
>+ * The process is put to sleep (TASK_UNINTERRUPTIBLE|TASK_NOLOAD) until
>the
>+ * @condition evaluates to true. The @condition is checked each time
>+ * the waitqueue @wq_head is woken up.
>+ *
>+ * wake_up() has to be called after changing any variable that could
>+ * change the result of the wait condition.
>+ *
>+ * This can be used instead of wait_event() when the event
>+ * being waited for is does not imply load on the system, but
>+ * when responding to signals is no appropriate, such as in
>+ * a kernel service thread.
>+ */
>+#define wait_event_noload(wq_head, condition)
>\
>+do {
>\
>+ might_sleep();
>\
>+ if (condition)
>\
>+ break;
>\
>+ __wait_event_noload(wq_head, condition);
Re: [lustre-devel] [PATCH 02/16] staging: lustre: replace simple cases of l_wait_event() with wait_event().
The wait calls in ll_statahead_thread are done in a service thread, and
should probably *not* contribute to load.
The one in osc_extent_wait is perhaps tough - It is called both from user
threads & daemon threads depending on the situation. The effect of adding
that to load average could be significant for some activities, even when
no user threads are busy. Thoughts from other Lustre people would be
welcome here.
Similar issues for osc_object_invalidate.
(If no one else speaks up, my vote is no contribution to load for those
OSC waits.)
Otherwise this one looks good...
On 12/18/17, 1:17 AM, "lustre-devel on behalf of NeilBrown"
wrote:
>@@ -968,7 +964,6 @@ static int ll_statahead_thread(void *arg)
> intfirst = 0;
> intrc = 0;
> struct md_op_data *op_data;
>- struct l_wait_info lwi= { 0 };
> sai = ll_sai_get(dir);
> sa_thread = &sai->sai_thread;
>@@ -1069,12 +1064,11 @@ static int ll_statahead_thread(void *arg)
> /* wait for spare statahead window */
> do {
>- l_wait_event(sa_thread->t_ctl_waitq,
>- !sa_sent_full(sai) ||
>- sa_has_callback(sai) ||
>- !list_empty(&sai->sai_agls) ||
>- !thread_is_running(sa_thread),
>- &lwi);
>+ wait_event(sa_thread->t_ctl_waitq,
>+ !sa_sent_full(sai) ||
>+ sa_has_callback(sai) ||
>+ !list_empty(&sai->sai_agls) ||
>+ !thread_is_running(sa_thread));
> sa_handle_callback(sai);
> spin_lock(&lli->lli_agl_lock);
>@@ -1128,11 +1122,10 @@ static int ll_statahead_thread(void *arg)
>* for file release to stop me.
>*/
> while (thread_is_running(sa_thread)) {
>- l_wait_event(sa_thread->t_ctl_waitq,
>- sa_has_callback(sai) ||
>- !agl_list_empty(sai) ||
>- !thread_is_running(sa_thread),
>- &lwi);
>+ wait_event(sa_thread->t_ctl_waitq,
>+ sa_has_callback(sai) ||
>+ !agl_list_empty(sai) ||
>+ !thread_is_running(sa_thread));
> sa_handle_callback(sai);
> }
>@@ -1145,9 +1138,8 @@ static int ll_statahead_thread(void *arg)
> CDEBUG(D_READA, "stop agl thread: sai %p pid %u\n",
> sai, (unsigned int)agl_thread->t_pid);
>- l_wait_event(agl_thread->t_ctl_waitq,
>- thread_is_stopped(agl_thread),
>- &lwi);
>+ wait_event(agl_thread->t_ctl_waitq,
>+ thread_is_stopped(agl_thread));
> } else {
> /* Set agl_thread flags anyway. */
> thread_set_flags(agl_thread, SVC_STOPPED);
>@@ -1159,8 +1151,8 @@ static int ll_statahead_thread(void *arg)
>*/
> while (sai->sai_sent != sai->sai_replied) {
> /* in case we're not woken up, timeout wait */
>- lwi = LWI_TIMEOUT(msecs_to_jiffies(MSEC_PER_SEC >> 3),
>-NULL, NULL);
>+ struct l_wait_info lwi =
>LWI_TIMEOUT(msecs_to_jiffies(MSEC_PER_SEC >>
>3),
>+ NULL, NULL);
> l_wait_event(sa_thread->t_ctl_waitq,
>sai->sai_sent == sai->sai_replied, &lwi);
> }
Re: [lustre-devel] [PATCH 04/16] staging: lustre: use wait_event_timeout() where appropriate.
Same thing as the other one, ll_statahead_thread should not contribute to
load.
IMO, mgc_process_log should not contribute to load in the case where it¹s
waiting for an import to recover, that¹s likely to be a pretty long wait
and doesn¹t really represent load. It¹s waiting for network recovery,
basically.
The OSC functions get the same question as the other ones - they happen
from user threads and from daemons. Curious again what Andreas and/or
Oleg think.
I think ptlrpc_reconnect_import should probably *not* contribute to load,
it¹s waiting for network recovery.
Same with ptlrpc_recover_import.
On 12/18/17, 1:17 AM, "lustre-devel on behalf of NeilBrown"
wrote:
>When the lwi arg has a timeout, but no timeout
>callback function, l_wait_event() acts much the same as
>wait_event_timeout() - the wait is not interruptible and
>simply waits for the event or the timeouts.
>
>The most noticable difference is that the return value is
>-ETIMEDOUT or 0, rather than 0 or non-zero.
>
>Another difference is that the process waiting is included in
>the load-average. This is probably more correct.
>
>A final difference is that if the timeout is zero, l_wait_event()
>will not time out at all. In the one case where that is possible
>we need to conditionally use wait_event_noload().
>
>So replace all such calls with wait_event_timeout(), being
>careful of the return value.
>
>In one case, there is no event, so use
>schedule_timeout_uninterruptible().
>
>
>Note that the presence or absence of LWI_ON_SIGNAL_NOOP
>has no effect in these cases.
>
>Signed-off-by: NeilBrown
>---
> drivers/staging/lustre/lustre/include/lustre_lib.h | 37
>
> drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 10 ++---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 12 ++
> drivers/staging/lustre/lustre/llite/statahead.c| 14 +++-
> drivers/staging/lustre/lustre/mdc/mdc_request.c|5 +--
> drivers/staging/lustre/lustre/mgc/mgc_request.c| 15 +++-
> drivers/staging/lustre/lustre/obdclass/cl_io.c | 17 +
> drivers/staging/lustre/lustre/osc/osc_cache.c | 25 ++
> drivers/staging/lustre/lustre/ptlrpc/events.c |7 +---
> drivers/staging/lustre/lustre/ptlrpc/import.c | 12 +++---
> .../staging/lustre/lustre/ptlrpc/pack_generic.c|9 ++---
> drivers/staging/lustre/lustre/ptlrpc/pinger.c | 12 ++
> drivers/staging/lustre/lustre/ptlrpc/recover.c | 12 +++---
> drivers/staging/lustre/lustre/ptlrpc/sec_gc.c | 10 ++---
> drivers/staging/lustre/lustre/ptlrpc/service.c | 10 ++---
> 15 files changed, 104 insertions(+), 103 deletions(-)
>
>diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h
>b/drivers/staging/lustre/lustre/include/lustre_lib.h
>index 08bdd618ea7d..fcf31c779e98 100644
>--- a/drivers/staging/lustre/lustre/include/lustre_lib.h
>+++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
>@@ -388,4 +388,41 @@ do {
>\
> schedule());
> \
> } while (0)
>
>+#define __wait_event_noload_timeout(wq_head, condition, timeout)
>\
>+ ___wait_event(wq_head, ___wait_cond_timeout(condition),
>\
>+(TASK_UNINTERRUPTIBLE | TASK_NOLOAD), 0, timeout,
>\
>+__ret = schedule_timeout(__ret))
>+
>+/**
>+ * wait_event_noload_timeout - sleep until a condition gets true or a
>timeout elapses
>+ * @wq_head: the waitqueue to wait on
>+ * @condition: a C expression for the event to wait for
>+ * @timeout: timeout, in jiffies
>+ *
>+ * The process is put to sleep (TASK_UNINTERRUPTIBLE | TASK_NOLOAD)
>until the
>+ * @condition evaluates to true. The @condition is checked each time
>+ * the waitqueue @wq_head is woken up.
>+ *
>+ * wake_up() has to be called after changing any variable that could
>+ * change the result of the wait condition.
>+ *
>+ * This is suitable for service threads that are waiting for work to do
>+ * where there is no implication that the event not being true yet
>implies
>+ * any load on the system, and where it is not appropriate for the
>+ * soft-lockup detector to warning if the wait is unusually long.
>+ *
>+ * Returns:
>+ * 0 if the @condition evaluated to %false after the @timeout elapsed,
>+ * 1 if the @condition evaluated to %true after the @timeout elapsed,
>+ * or the remaining jiffies (at least 1) if the @condition evaluated
>+ * to %true before the @timeout elapsed.
>+ */
>+#define wait_event_noload_timeout(wq_head, condition, timeout)
>\
>+({
>\
>+ long __ret = timeout;
>\
>+ might_sleep();
>\
>+ if (!___wait_cond_timeout(condition))
Re: [lustre-devel] [PATCH 08/16] staging: lustre: open code polling loop instead of using l_wait_event()
The lov_check_and_wait_active wait is usually (always?) going to be
asynchronous from userspace and probably shouldn¹t contribute to load.
So I guess that means schedule_timeout_idle.
On 12/18/17, 1:18 AM, "lustre-devel on behalf of NeilBrown"
wrote:
>Two places that LWI_TIMEOUT_INTERVAL() is used, the outcome is a
>simple polling loop that polls every second for some event (with a
>limit).
>
>So write a simple loop to make this more apparent.
>
>Signed-off-by: NeilBrown
>---
> drivers/staging/lustre/lustre/llite/llite_lib.c | 11 +--
> drivers/staging/lustre/lustre/lov/lov_request.c | 12 +---
> 2 files changed, 10 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c
>b/drivers/staging/lustre/lustre/llite/llite_lib.c
>index 33dc15e9aebb..f6642fa30428 100644
>--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
>+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
>@@ -1984,8 +1984,7 @@ void ll_umount_begin(struct super_block *sb)
> struct ll_sb_info *sbi = ll_s2sbi(sb);
> struct obd_device *obd;
> struct obd_ioctl_data *ioc_data;
>- wait_queue_head_t waitq;
>- struct l_wait_info lwi;
>+ int cnt = 0;
>
> CDEBUG(D_VFSTRACE, "VFS Op: superblock %p count %d active %d\n", sb,
> sb->s_count, atomic_read(&sb->s_active));
>@@ -2021,10 +2020,10 @@ void ll_umount_begin(struct super_block *sb)
>* and then continue. For now, we just periodically checking for vfs
>* to decrement mnt_cnt and hope to finish it within 10sec.
>*/
>- init_waitqueue_head(&waitq);
>- lwi = LWI_TIMEOUT_INTERVAL(10 * HZ,
>- HZ, NULL, NULL);
>- l_wait_event(waitq, may_umount(sbi->ll_mnt.mnt), &lwi);
>+ while (cnt < 10 && !may_umount(sbi->ll_mnt.mnt)) {
>+ schedule_timeout_uninterruptible(HZ);
>+ cnt ++;
>+ }
>
> schedule();
> }
>diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c
>b/drivers/staging/lustre/lustre/lov/lov_request.c
>index fb3b7a7fa32a..c1e58fcc30b3 100644
>--- a/drivers/staging/lustre/lustre/lov/lov_request.c
>+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
>@@ -99,8 +99,7 @@ static int lov_check_set(struct lov_obd *lov, int idx)
> */
> static int lov_check_and_wait_active(struct lov_obd *lov, int ost_idx)
> {
>- wait_queue_head_t waitq;
>- struct l_wait_info lwi;
>+ int cnt = 0;
> struct lov_tgt_desc *tgt;
> int rc = 0;
>
>@@ -125,11 +124,10 @@ static int lov_check_and_wait_active(struct lov_obd
>*lov, int ost_idx)
>
> mutex_unlock(&lov->lov_lock);
>
>- init_waitqueue_head(&waitq);
>- lwi = LWI_TIMEOUT_INTERVAL(obd_timeout * HZ,
>- HZ, NULL, NULL);
>-
>- rc = l_wait_event(waitq, lov_check_set(lov, ost_idx), &lwi);
>+ while (cnt < obd_timeout && !lov_check_set(lov, ost_idx)) {
>+ schedule_timeout_uninterruptible(HZ);
>+ cnt ++;
>+ }
> if (tgt->ltd_active)
> return 1;
>
>
>
>___
>lustre-devel mailing list
>lustre-de...@lists.lustre.org
>http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Re: [lustre-devel] [PATCH 11/16] staging: lustre: make polling loop in ptlrpc_unregister_bulk more obvious
This probably shouldn¹t contribute to load, it¹s often (mostly?) run out
of the ptlrpcd daemons.
- Patrick
On 12/18/17, 1:18 AM, "lustre-devel on behalf of NeilBrown"
wrote:
>This use of l_wait_event() is a polling loop that re-checks
>every second. Make this more obvious with a while loop
>and wait_event_timeout().
>
>Signed-off-by: NeilBrown
>---
> drivers/staging/lustre/lustre/ptlrpc/niobuf.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/staging/lustre/lustre/ptlrpc/niobuf.c
>b/drivers/staging/lustre/lustre/ptlrpc/niobuf.c
>index 0c2ded721c49..5606c8f01b5b 100644
>--- a/drivers/staging/lustre/lustre/ptlrpc/niobuf.c
>+++ b/drivers/staging/lustre/lustre/ptlrpc/niobuf.c
>@@ -229,7 +229,6 @@ int ptlrpc_unregister_bulk(struct ptlrpc_request
>*req, int async)
> {
> struct ptlrpc_bulk_desc *desc = req->rq_bulk;
> wait_queue_head_t *wq;
>- struct l_wait_info lwi;
> int rc;
>
> LASSERT(!in_interrupt()); /* might sleep */
>@@ -246,7 +245,7 @@ int ptlrpc_unregister_bulk(struct ptlrpc_request
>*req, int async)
>
> /* the unlink ensures the callback happens ASAP and is the last
>* one. If it fails, it must be because completion just happened,
>- * but we must still l_wait_event() in this case to give liblustre
>+ * but we must still wait_event() in this case to give liblustre
>* a chance to run client_bulk_callback()
>*/
> mdunlink_iterate_helper(desc->bd_mds, desc->bd_md_max_brw);
>@@ -270,15 +269,16 @@ int ptlrpc_unregister_bulk(struct ptlrpc_request
>*req, int async)
> /* Network access will complete in finite time but the HUGE
>* timeout lets us CWARN for visibility of sluggish LNDs
>*/
>- lwi = LWI_TIMEOUT_INTERVAL(LONG_UNLINK * HZ,
>- HZ, NULL, NULL);
>- rc = l_wait_event(*wq, !ptlrpc_client_bulk_active(req), &lwi);
>- if (rc == 0) {
>+ int cnt = 0;
>+ while (cnt < LONG_UNLINK &&
>+ (rc = wait_event_timeout(*wq,
>!ptlrpc_client_bulk_active(req),
>+ HZ)) == 0)
>+ cnt += 1;
>+ if (rc > 0) {
> ptlrpc_rqphase_move(req, req->rq_next_phase);
> return 1;
> }
>
>- LASSERT(rc == -ETIMEDOUT);
> DEBUG_REQ(D_WARNING, req, "Unexpectedly long timeout: desc %p",
> desc);
> }
>
>
>___
>lustre-devel mailing list
>lustre-de...@lists.lustre.org
>http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Re: [lustre-devel] [PATCH 15/16] staging: lustre: use explicit poll loop in ptlrpc_unregister_reply
This should not contribute to load, since it¹s called out of the ptlrpcd
daemons.
On 12/18/17, 1:18 AM, "lustre-devel on behalf of NeilBrown"
wrote:
>replace l_wait_event() with wait_event_timeout() and explicit
>loop. This approach is easier to understand.
>
>Signed-off-by: NeilBrown
>---
> drivers/staging/lustre/lustre/ptlrpc/client.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/staging/lustre/lustre/ptlrpc/client.c
>b/drivers/staging/lustre/lustre/ptlrpc/client.c
>index 3e6d22beb9f5..bb8c9ab68f5f 100644
>--- a/drivers/staging/lustre/lustre/ptlrpc/client.c
>+++ b/drivers/staging/lustre/lustre/ptlrpc/client.c
>@@ -2500,7 +2500,6 @@ static int ptlrpc_unregister_reply(struct
>ptlrpc_request *request, int async)
> {
> int rc;
> wait_queue_head_t *wq;
>- struct l_wait_info lwi;
>
> /* Might sleep. */
> LASSERT(!in_interrupt());
>@@ -2543,16 +2542,17 @@ static int ptlrpc_unregister_reply(struct
>ptlrpc_request *request, int async)
>* Network access will complete in finite time but the HUGE
>* timeout lets us CWARN for visibility of sluggish NALs
>*/
>- lwi = LWI_TIMEOUT_INTERVAL(LONG_UNLINK * HZ,
>- HZ, NULL, NULL);
>- rc = l_wait_event(*wq, !ptlrpc_client_recv_or_unlink(request),
>-&lwi);
>- if (rc == 0) {
>+ int cnt = 0;
>+ while (cnt < LONG_UNLINK &&
>+ (rc = wait_event_timeout(*wq,
>+
>!ptlrpc_client_recv_or_unlink(request),
>+ HZ)) == 0)
>+ cnt += 1;
>+ if (rc > 0) {
> ptlrpc_rqphase_move(request, request->rq_next_phase);
> return 1;
> }
>
>- LASSERT(rc == -ETIMEDOUT);
> DEBUG_REQ(D_WARNING, request,
> "Unexpectedly long timeout receiving_reply=%d
> req_ulinked=%d
>reply_unlinked=%d",
> request->rq_receiving_reply,
>
>
>___
>lustre-devel mailing list
>lustre-de...@lists.lustre.org
>http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
