Re: [PATCH] target: Fix a double put in transport_free_session
Dne 23. 03. 21 v 3:58 Lv Yunlong napsal(a):
> In transport_free_session, se_nacl is got from se_sess
> with the initial reference. If se_nacl->acl_sess_list is
> empty, se_nacl->dynamic_stop is set to true. Then the first
> target_put_nacl(se_nacl) will drop the initial reference
> and free se_nacl. Later there is a second target_put_nacl()
> to put se_nacl. It may cause error in race.
>
> My patch sets se_nacl->dynamic_stop to false to avoid the
> double put.
>
> Signed-off-by: Lv Yunlong
> ---
> drivers/target/target_core_transport.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_transport.c
> b/drivers/target/target_core_transport.c
> index 5ecb9f18a53d..c266defe694f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -584,8 +584,10 @@ void transport_free_session(struct se_session *se_sess)
> }
> mutex_unlock(&se_tpg->acl_node_mutex);
>
> - if (se_nacl->dynamic_stop)
> + if (se_nacl->dynamic_stop) {
> target_put_nacl(se_nacl);
> + se_nacl->dynamic_stop = false;
> + }
>
> target_put_nacl(se_nacl);
> }
>
FYI,
I have received a bug report against the 5.8 kernel about task hangs that seems
to involve the nacl "dynamic_stop" code
Mar 4 16:49:44 gzboot kernel: [186322.177819] INFO: task targetcli:2359053
blocked for more than 120 seconds.
Mar 4 16:49:44 gzboot kernel: [186322.178862] Tainted: P O
5.8.0-44-generic #50-Ubuntu
Mar 4 16:49:44 gzboot kernel: [186322.179746] "echo 0 >
/proc/sys/kernel/hung_task_timeout_secs" disables this message.
Mar 4 16:49:44 gzboot kernel: [186322.180583] targetcli D0 2359053
2359052 0x
Mar 4 16:49:44 gzboot kernel: [186322.180586] Call Trace:
Mar 4 16:49:44 gzboot kernel: [186322.180592] __schedule+0x212/0x5d0
Mar 4 16:49:44 gzboot kernel: [186322.180595] ? usleep_range+0x90/0x90
Mar 4 16:49:44 gzboot kernel: [186322.180596] schedule+0x55/0xc0
Mar 4 16:49:44 gzboot kernel: [186322.180597] schedule_timeout+0x10f/0x160
Mar 4 16:49:44 gzboot kernel: [186322.180601] ? evict+0x14c/0x1b0
Mar 4 16:49:44 gzboot kernel: [186322.180602] __wait_for_common+0xa8/0x150
Mar 4 16:49:44 gzboot kernel: [186322.180603] wait_for_completion+0x24/0x30
Mar 4 16:49:44 gzboot kernel: [186322.180637]
core_tpg_del_initiator_node_acl+0x8e/0x120 [target_core_mod]
Mar 4 16:49:44 gzboot kernel: [186322.180643]
target_fabric_nacl_base_release+0x26/0x30 [target_core_mod]
Mar 4 16:49:44 gzboot kernel: [186322.180647] config_item_cleanup+0x5d/0xf0
Mar 4 16:49:44 gzboot kernel: [186322.180649] config_item_put+0x2d/0x40
Mar 4 16:49:44 gzboot kernel: [186322.180651] configfs_rmdir+0x1d8/0x350
Mar 4 16:49:44 gzboot kernel: [186322.180653] vfs_rmdir.part.0+0x66/0x190
Mar 4 16:49:44 gzboot kernel: [186322.180654] do_rmdir+0x1b4/0x200
Mar 4 16:49:44 gzboot kernel: [186322.180655] __x64_sys_rmdir+0x17/0x20
Mar 4 16:49:44 gzboot kernel: [186322.180657] do_syscall_64+0x49/0xc0
Mar 4 16:49:44 gzboot kernel: [186322.180659]
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Mar 4 16:49:44 gzboot kernel: [186322.180660] RIP: 0033:0x7f30cf1ca9eb
Mar 4 16:49:44 gzboot kernel: [186322.180661] RSP: 002b:7ffd72030bd8
EFLAGS: 0246 ORIG_RAX: 0054
Mar 4 16:49:44 gzboot kernel: [186322.180662] RAX: ffda RBX:
ff9c RCX: 7f30cf1ca9eb
Mar 4 16:49:44 gzboot kernel: [186322.180663] RDX: RSI:
RDI: 7f30cc1e2a50
Mar 4 16:49:44 gzboot kernel: [186322.180664] RBP: 00a4b7a0 R08:
R09: 7ffd72030b7f
Mar 4 16:49:44 gzboot kernel: [186322.180664] R10: R11:
0246 R12: 7ffd72030c00
Mar 4 16:49:44 gzboot kernel: [186322.180665] R13: 7f30cdd706a8 R14:
7f30ced00cc0 R15: 7f30cdd70698
Maurizio
Re: [PATCH] scsi: target: remove redundant assignment to variable 'ret'
Dne 14. 09. 20 v 4:32 Jing Xiangfeng napsal(a):
> The variable ret has been initialized with a value '0'. The assignment
> in switch-case is redundant. So remove it.
>
> Signed-off-by: Jing Xiangfeng
> ---
> drivers/target/iscsi/iscsi_target.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c
> b/drivers/target/iscsi/iscsi_target.c
> index cd045dc75a58..f5272ac18b16 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -4516,7 +4516,6 @@ int iscsit_logout_post_handler(
> iscsit_logout_post_handler_closesession(conn);
> break;
> }
> - ret = 0;
> break;
> case ISCSI_LOGOUT_REASON_CLOSE_CONNECTION:
> if (conn->cid == cmd->logout_cid) {
> @@ -4527,7 +4526,6 @@ int iscsit_logout_post_handler(
> iscsit_logout_post_handler_samecid(conn);
> break;
> }
> - ret = 0;
> } else {
> switch (cmd->logout_response) {
> case ISCSI_LOGOUT_SUCCESS:
>
Looks ok to me.
Reviewed-by: Maurizio Lombardi
Re: [PATCH] fs: exFAT read-only driver GPL implementation by Paragon Software.
Dne 21.10.2019 v 13:45 Pali Rohár napsal(a): > They are represented by one member > in boot sector structure). > >> Btw, only Windows CE supported this. > > Is this information based on some real tests? Or just from marketing or > Microsoft's information? (I would really like to know definite answer in > this area). I admit I have read it on Microsoft's exFAT documentation, unfortunately I don't have a WinCE device to test if it's really true. Maurizio
Re: [PATCH] fs: exFAT read-only driver GPL implementation by Paragon Software.
Dne 21.10.2019 v 13:11 Pali Rohár napsal(a): > Are you going to add support also for TexFAT? Or at least for more two > FAT tables (like is used in FAT32)? > Just a small note here, differences between FAT and exFAT: 1) Contiguous files get a special treatment by exFAT: they do not use the FAT cluster chain. 2) exFAT doesn't use the FAT to track free space, it uses a bitmap. So, 2 FAT tables are probably not sufficient for recovery, 2 bitmaps are needed too.[1] Btw, only Windows CE supported this. [1] http://www.ntfs.com/exfat-allocation-bitmap.htm Maurizio
Re: [PATCH] fs: exFAT read-only driver GPL implementation by Paragon Software.
Dne 21.10.2019 v 12:54 Pali Rohár napsal(a): > Plus there is new version of > this out-of-tree Samsung's exfat driver called sdfat which can be found > in some Android phones. [...] > > About that one implementation from Samsung, which was recently merged > into staging tree, more people wrote that code is in horrible state and > probably it should not have been merged. That implementation has > all-one-one driver FAT12, FAT16, FAT32 and exFAT which basically > duplicate current kernel fs/fat code. > > Quick look at this Konstantin's patch, it looks like that code is not in > such bad state as staging one. It has only exFAT support (no FAT32) but > there is no write support (yet). But, AFAIK, Samsung is preparing a patch that will replace the current staging driver with their newer sdfat driver that also has write support. https://marc.info/?l=linux-fsdevel&m=156985252507812&w=2 Maurizio
Re: [PATCH] bnx2fc: reduce stack usage in __bnx2fc_enable
Hi, On 10/07/2015 03:11 PM, Arnd Bergmann wrote: > - memset(&npiv_tbl, 0, sizeof(npiv_tbl)); > - if (hba->cnic->get_fc_npiv_tbl(hba->cnic, &npiv_tbl)) > + npiv_tbl = kzalloc(sizeof(struct cnic_fc_npiv_tbl), GFP_KERNEL); > + if (!npiv_tbl) > goto done; > If kzalloc() fails perhaps the function should return -ENOMEM, not zero. Regards, Maurizio Lombardi -- 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: [RFC PATCH 0/3] fix *pbl format support
Hi Rasmus,
On 09/16/2015 10:35 PM, Rasmus Villemoes wrote:
>
> I just remembered: I noticed a while ago that the qualifier member is
> only used inside format_decode (in the end, the information is folded
> into the type member), so one might as well use a local variable for
> that. This gives option (3): Make field_width a 24 bit bitfield. I think
> that should be sufficient for any realistic bitmap that one would
> print. The patch is also rather small. Surprisingly, bloat-o-meter even
> says that it's also a net win in code size:
I tested your patch with scsi-debug and it works for me:
# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1
# cat /sys/bus/pseudo/drivers/scsi_debug/map
# vgcreate tsvg /dev/sdb
Physical volume "/dev/sdb" successfully created
Volume group "tsvg" successfully created
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15
# lvcreate -V200m -l99%FREE -T tsvg/pool -n lv1 --discards ignore
Logical volume "lv1" created.
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-31,2048-2055,501760-501871
Thanks,
Maurizio Lombardi
>
> add/remove: 18/16 grow/shrink: 3/2 up/down: 5551/-5775 (-224)
>
> (the huge absolute numbers are due to stuff like
>
> pointer-1148 +1148
> pointer.isra1116 - -1116
>
> so the functions haven't actually changed that much). I'll have to check
> how this can be (smaller might still be worse), but at least it doesn't
> seem to be a catastrophe in terms of .text bloat.
>
> [Grr, why don't we have a way to do a compile-time assert outside
> function context?]
>
> Only compile-tested.
>
> From: Rasmus Villemoes
> Date: Wed, 16 Sep 2015 22:06:14 +0200
> Subject: [RFC] lib/vsprintf.c: expand field_width to 24 bits
>
> Maurizio Lombardi reported a problem with the %pb extension: It
> doesn't work for sufficiently large bitmaps, since the size is stashed
> in the field_width field of the struct printf_spec, which is currently
> an s16. Concretely, this manifested itself in
> /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
> printer got a size of 0, which is the 16 bit truncation of the actual
> bitmap size.
>
> We do want to keep struct printf_spec at 8 bytes so that it can
> cheaply be passed by value. The qualifier field is only used for
> internal bookkeeping in format_decode, so we might as well use a local
> variable for that. This gives us an additional 8 bits, which we can
> then use for the field width. To stay in 8 bytes, we need to do a
> little rearranging and make the type member a bitfield as well.
>
> Signed-off-by: Rasmus Villemoes
> ---
> lib/vsprintf.c | 33 +
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 95cd63b43b99..cce2a780a82e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -380,13 +380,13 @@ enum format_type {
> };
>
> struct printf_spec {
> - u8 type; /* format_type enum */
> + u8 type:8; /* format_type enum */
> + s32 field_width:24; /* width of output field */
> u8 flags; /* flags to number() */
> u8 base; /* number base, 8, 10 or 16 only */
> - u8 qualifier; /* number qualifier, one of 'hHlLtzZ' */
> - s16 field_width;/* width of output field */
> s16 precision; /* # of digits/chars */
> };
> +extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
>
> static noinline_for_stack
> char *number(char *buf, char *end, unsigned long long num,
> @@ -1633,6 +1633,7 @@ static noinline_for_stack
> int format_decode(const char *fmt, struct printf_spec *spec)
> {
> const char *start = fmt;
> + char qualifier;
>
> /* we finished early by reading the field width */
> if (spec->type == FORMAT_TYPE_WIDTH) {
> @@ -1715,16 +1716,16 @@ precision:
>
> qualifier:
> /* get the conversion qualifier */
> - spec->qualifier = -1;
> + qualifier = 0;
> if (*fmt == 'h' || _tolower(*fmt) == 'l' ||
> _tolower(*fmt) == 'z' || *fmt == 't') {
> - spec->qualifier = *fmt++;
> - if (unlikely(spec->qualifier == *fmt)) {
> - if (spec->qualifier == 'l') {
> - spec->qualifier = 'L';
> + qualifier = *fmt++;
> + if (unlikely(qualifier == *fmt)) {
> + if (qualifier == 'l') {
> +
Re: [RFC PATCH 0/3] fix *pbl format support
Hi, On 09/16/2015 02:27 PM, Rasmus Villemoes wrote: > > If we want to fix the problem with 3/3, then this seems obviously > necessary. There may be stuff we want to optimize later (for example, I > don't think we should always make a local copy of the entire struct; Yes I know, I just tried not to break anything in the process, optimizations can be done later. > I haven't looked carefully at your code, but it does seem that you make > sure that at least the return value is as expected, which will make > kasprintf work. But it seems there is another kasprintf > problem. [reminder: kasprintf works by doing a va_copy, then doing a > first call of vsnprintf, passing NULL for the buffer and 0 for the > length to determine the size to allocate, and then doing the actual > formatting with a second call] Ah, you're right, PATCH 2 is broken because I didn't think to the case you described. Please ignore it, thanks for catching this. > I'm not yet completely convinced this is the right solution. Obviously, > if other problems with the small .field_width size show up, this might > be necessary, but as long as it's only the %pb formatter (and so far > only a single user of that), I think smaller/other hammers should be > thought about. So far I think there've been two alternatives: (1) > reintroduce the dedicated bitmap pretty printer(s) I have no problem with that, at least it will work again. Thanks for the review, Maurizio Lombardi -- 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/
[RFC PATCH 2/3] lib/vsprintf.c: append "..." if the *pb[l] output has been truncated.
The *pb[l] format may generate a very long string that could exaust
the output buffer capacity;
when such event happens the output could be misleading,
it may appear valid but part of it has been truncated.
This patch modifies the bitmap_*_string() functions so they will append
"..." to the output to inform the user that a truncation happened.
Signed-off-by: Maurizio Lombardi
---
lib/vsprintf.c | 48
1 file changed, 48 insertions(+)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8707d91..f49bf54 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -816,6 +816,7 @@ char *bitmap_string(char *buf, char *end, unsigned long
*bitmap,
int i, chunksz;
bool first = true;
struct printf_spec spec = *specp;
+ const char *buf_start = buf;
/* reused to print numbers */
spec = (struct printf_spec){ .flags = SMALL | ZEROPAD, .base = 16 };
@@ -846,6 +847,29 @@ char *bitmap_string(char *buf, char *end, unsigned long
*bitmap,
chunksz = CHUNKSZ;
}
+
+ if (buf >= end && buf_start != end) {
+ int spc = 0;
+ char *trunc = end - 1;
+
+ while (trunc > buf_start) {
+ if (*trunc == ',' && spc > 3) {
+ trunc++;
+ break;
+ }
+ trunc--;
+ spc++;
+ }
+
+ if (spc > 3) {
+ trunc[0] = '.';
+ trunc[1] = '.';
+ trunc[2] = '.';
+ trunc[3] = '\0';
+ } else
+ trunc[0] = '\0';
+ }
+
return buf;
}
@@ -858,6 +882,7 @@ char *bitmap_list_string(char *buf, char *end, unsigned
long *bitmap,
int cur, rbot, rtop;
bool first = true;
struct printf_spec spec = *specp;
+ const char *buf_start = buf;
/* reused to print numbers */
spec = (struct printf_spec){ .base = 10 };
@@ -887,6 +912,29 @@ char *bitmap_list_string(char *buf, char *end, unsigned
long *bitmap,
rbot = cur;
}
+
+ if (buf >= end && buf_start != end) {
+ int spc = 0;
+ char *trunc = end - 1;
+
+ while (trunc > buf_start) {
+ if (*trunc == ',' && spc > 3) {
+ trunc++;
+ break;
+ }
+ trunc--;
+ spc++;
+ }
+
+ if (spc > 3) {
+ trunc[0] = '.';
+ trunc[1] = '.';
+ trunc[2] = '.';
+ trunc[3] = '\0';
+ } else
+ trunc[0] = '\0';
+ }
+
return buf;
}
--
Maurizio Lombardi
--
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/
[RFC PATCH 3/3] lib/vsprintf.c: increase the size of the field_width variable
When printing a bitmap using the "%*pb[l]" printk format
a 16 bit variable (field_width) is used to store the size of the bitmap.
In some cases 16 bits are not sufficient, the variable overflows and
printk does not work as expected.
This patch fixes the problem by changing the type of field_width to s32.
How to reproduce the bug:
1.load scsi_debug
# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1
2.create VG
# vgcreate tsvg /dev/sdb
Physical volume "/dev/sdb" successfully created
Volume group "tsvg" successfully created
3. Bitmap should be set, but still empty
# cat /sys/bus/pseudo/drivers/scsi_debug/map
Expected results:
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15
Signed-off-by: Maurizio Lombardi
---
lib/vsprintf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f49bf54..9712260 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -384,8 +384,8 @@ struct printf_spec {
u8 flags; /* flags to number() */
u8 base; /* number base, 8, 10 or 16 only */
u8 qualifier; /* number qualifier, one of 'hHlLtzZ' */
- s16 field_width;/* width of output field */
s16 precision; /* # of digits/chars */
+ s32 field_width;/* width of output field */
};
static noinline_for_stack
--
Maurizio Lombardi
--
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/
[RFC PATCH 1/3] lib/vsprintf.c: Do not pass printf_spec by value on stack.
The original code passes the structure by value on the stack,
this limits the size of the printf_spec structure because of
performance reasons.
This patch modifies the code so only a const pointer to the structure
is passed on the stack.
Signed-off-by: Maurizio Lombardi
---
lib/vsprintf.c | 225 ++---
1 file changed, 118 insertions(+), 107 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b..8707d91 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -390,9 +390,10 @@ struct printf_spec {
static noinline_for_stack
char *number(char *buf, char *end, unsigned long long num,
-struct printf_spec spec)
+const struct printf_spec *specp)
{
/* put_dec requires 2-byte alignment of the buffer. */
+ struct printf_spec spec = *specp;
char tmp[3 * sizeof(num)] __aligned(2);
char sign;
char locase;
@@ -508,9 +509,10 @@ char *number(char *buf, char *end, unsigned long long num,
}
static noinline_for_stack
-char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+char *string(char *buf, char *end, const char *s, const struct printf_spec
*specp)
{
int len, i;
+ struct printf_spec spec = *specp;
if ((unsigned long)s < PAGE_SIZE)
s = "(null)";
@@ -557,7 +559,7 @@ static void widen(char *buf, char *end, unsigned len,
unsigned spaces)
}
static noinline_for_stack
-char *dentry_name(char *buf, char *end, const struct dentry *d, struct
printf_spec spec,
+char *dentry_name(char *buf, char *end, const struct dentry *d, const struct
printf_spec *specp,
const char *fmt)
{
const char *array[4], *s;
@@ -585,7 +587,7 @@ char *dentry_name(char *buf, char *end, const struct dentry
*d, struct printf_sp
}
}
s = array[--i];
- for (n = 0; n != spec.precision; n++, buf++) {
+ for (n = 0; n != specp->precision; n++, buf++) {
char c = *s++;
if (!c) {
if (!i)
@@ -597,10 +599,10 @@ char *dentry_name(char *buf, char *end, const struct
dentry *d, struct printf_sp
*buf = c;
}
rcu_read_unlock();
- if (n < spec.field_width) {
+ if (n < specp->field_width) {
/* we want to pad the sucker */
- unsigned spaces = spec.field_width - n;
- if (!(spec.flags & LEFT)) {
+ unsigned spaces = specp->field_width - n;
+ if (!(specp->flags & LEFT)) {
widen(buf - n, end, n, spaces);
return buf + spaces;
}
@@ -615,9 +617,10 @@ char *dentry_name(char *buf, char *end, const struct
dentry *d, struct printf_sp
static noinline_for_stack
char *symbol_string(char *buf, char *end, void *ptr,
- struct printf_spec spec, const char *fmt)
+ const struct printf_spec *specp, const char *fmt)
{
unsigned long value;
+ struct printf_spec spec = *specp;
#ifdef CONFIG_KALLSYMS
char sym[KSYM_SYMBOL_LEN];
#endif
@@ -634,19 +637,19 @@ char *symbol_string(char *buf, char *end, void *ptr,
else
sprint_symbol_no_offset(sym, value);
- return string(buf, end, sym, spec);
+ return string(buf, end, sym, &spec);
#else
spec.field_width = 2 * sizeof(void *);
spec.flags |= SPECIAL | SMALL | ZEROPAD;
spec.base = 16;
- return number(buf, end, value, spec);
+ return number(buf, end, value, &spec);
#endif
}
static noinline_for_stack
char *resource_string(char *buf, char *end, struct resource *res,
- struct printf_spec spec, const char *fmt)
+ const struct printf_spec *specp, const char *fmt)
{
#ifndef IO_RSRC_PRINTK_SIZE
#define IO_RSRC_PRINTK_SIZE6
@@ -700,73 +703,73 @@ char *resource_string(char *buf, char *end, struct
resource *res,
char *p = sym, *pend = sym + sizeof(sym);
int decode = (fmt[0] == 'R') ? 1 : 0;
- const struct printf_spec *specp;
+ const struct printf_spec *sp;
*p++ = '[';
if (res->flags & IORESOURCE_IO) {
- p = string(p, pend, "io ", str_spec);
- specp = &io_spec;
+ p = string(p, pend, "io ", &str_spec);
+ sp = &io_spec;
} else if (res->flags & IORESOURCE_MEM) {
- p = string(p, pend, "mem ", str_spec);
- specp = &mem_spec;
+ p = string(p, pend, "mem ", &str_spec);
+ sp = &mem_spec;
} else if (res->flags & IORESOURCE_IRQ) {
- p = string(p, pend, "irq ", str_spec);
- specp = &dec_spec;
+ p = stri
[RFC PATCH 0/3] fix *pbl format support
Hi, I tried to fix the "*pb[l]" format issue while taking care of the problems discussed in this thread: https://lkml.org/lkml/2015/9/9/153 I would like to know whether this approach is more acceptable to you: PATCH 1 modifies the code so that the printf_spec struct is not passed by value anymore, instead a const pointer is used and the structure is copied to a local variable only when necessary. PATCH 2 modifies the bitmap_*_string() functions so they'll append "..." to the output string whenever the buffer is not sufficiently large. example of output: *pb: ,... *pbl: 1-2,5-7,... PATCH 3 increases the size of printf_spec.field_width (from s16 to s32). Maurizio Lombardi (3): lib/vsprintf.c: Do not pass printf_spec by value on stack. lib/vsprintf.c: append "..." if the *pb[l] output has been truncated. lib/vsprintf.c: increase the size of the field_width variable lib/vsprintf.c | 275 +++-- 1 file changed, 167 insertions(+), 108 deletions(-) -- Maurizio Lombardi -- 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] lib/vsprintf.c: increase the size of the field_width variable
On 09/10/2015 09:38 AM, Joe Perches wrote:ing will be printed. > > Maurizio, did you try the patch I posted? > I think it'll work, but it doesn't fix the > fundamental issue of %*pbl with large bitmaps. I tested your patch, it works for the simple cases where bits are set below the S16_MAX limit, example: # modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1 # vgcreate tsvg /dev/sdb # cat /sys/bus/pseudo/drivers/scsi_debug/map 0-15 <--- OK! # lvcreate -V200m -l99%FREE -T tsvg/pool -n lv1 --discards ignore # cat /sys/bus/pseudo/drivers/scsi_debug/map 0-31,2048-2055< NO! It should be "0-31,2048-2055,501760-501871"! I think it's misleading. Can I suggest as a temporary solution to restore the old bitmap_scnlistprintf() function? > > Perhaps the thin wrapper conversions in lib/bitmap.c > in that commit for bitmap_scnprintf, bscnl_emit, and > bitmap_scnlistprintf should be reverted. > > Thanks, Maurizio Lombardi -- 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] lib/vsprintf.c: increase the size of the field_width variable
On 09/10/2015 09:04 AM, Maurizio Lombardi wrote:
>
> scsi-debug used the bitmap_scnlistprintf() function but since commit
> dbc760bcc150cc27160f0131b15db76350df4334 this function is just a wrapper
> around scnprintf("%*pbl");
I just want to add that since commit
46385326cc1577587ed3e7432c2425cf6d3e4308 the bitmap_scnlistprintf()
function has been completely removed because " all the bitmap formatting
usages have been converted to '%*pb[l]' ".
Thanks,
Maurizio Lombardi
--
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] lib/vsprintf.c: increase the size of the field_width variable
Hi Rasmus,
On 09/09/2015 08:51 PM, Rasmus Villemoes wrote:
> I'm also a little confused; I don't see what printk has to do with the
> reported problem (I'd expect the /sys/... file to be generated by
> something like seq_printf).
In the scsi-debug case scnprintf is used, but it doesn't really matter
because the change I made would influence printk and all its friends as
well... everything that will parse "%*pb[l]".
>
>> %*pb is meant for smallish bitmaps, not big ones.
>
> Yup. And that leads to my other confusion: Given that the expected
> output is given as "0-15", does the bitmap really consist of > S16_MAX
> bits with only the first 16 set?
>
Yes. To be precise, in the example I mentioned in the commit message, a
bitmap of size = 524288 bits is created.
If you assign this number to a s16 variable the result will be zero and
nothing will be printed.
Joe, you mentioned that *pbl is meant for small bitmaps, what should I
use with big ones?
scsi-debug used the bitmap_scnlistprintf() function but since commit
dbc760bcc150cc27160f0131b15db76350df4334 this function is just a wrapper
around scnprintf("%*pbl");
as a consequence, the scsi-debug map_show() function stopped working
correctly.
Thanks,
Maurizio Lombardi
--
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/
[PATCH] lib/vsprintf.c: increase the size of the field_width variable
When printing a bitmap using the "%*pb[l]" printk format
a 16 bit variable (field_width) is used to store the size of the bitmap.
In some cases 16 bits are not sufficient, the variable overflows and
printk does not work as expected.
This patch fixes the problem by changing the type of field_width to s32.
How to reproduce the bug:
1.load scsi_debug
# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1
2.create VG
# vgcreate tsvg /dev/sdb
Physical volume "/dev/sdb" successfully created
Volume group "tsvg" successfully created
3. Bitmap should be set, but still empty
# cat /sys/bus/pseudo/drivers/scsi_debug/map
Expected results:
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15
Signed-off-by: Maurizio Lombardi
---
lib/vsprintf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b..20f91c4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -384,8 +384,8 @@ struct printf_spec {
u8 flags; /* flags to number() */
u8 base; /* number base, 8, 10 or 16 only */
u8 qualifier; /* number qualifier, one of 'hHlLtzZ' */
- s16 field_width;/* width of output field */
s16 precision; /* # of digits/chars */
+ s32 field_width;/* width of output field */
};
static noinline_for_stack
--
Maurizio Lombardi
--
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/
[PATCH V4] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum
number of segments has been reached, regardless of the fact the page we
are going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation
conditions, a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page only in case
the latter starts a new segment and the maximum number of segments has
already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing `/dev/st0': Device or resource busy
V2: restore the correct number of segments in case of failure.
V3: In case of error, V2 restored the previous number of segments but left
the BIO_SEG_VALID flag set.
To avoid problems, after the page is removed from the bio vec,
V3 performs a recount of the segments in the error code path.
[ming@canonical.com: update bi_iter.bi_size before recounting segments]
V4: merge_bvec_fn() must be called with the old bi_iter.bi_size value.
Signed-off-by: Maurizio Lombardi
Signed-off-by: Ming Lei
---
block/bio.c | 54 ++
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 3e6331d..535b16d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -745,6 +745,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
}
}
+ bio->bi_iter.bi_size += len;
goto done;
}
@@ -761,29 +762,32 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bio->bi_phys_segments++;
+ bio->bi_iter.bi_size += len;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -792,7 +796,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
struct bvec_merge_data bvm = {
.bi_bdev = bio->bi_bdev,
.bi_sector = bio->bi_iter.bi_sector,
- .bi_size = bio->bi_iter.bi_size,
+ .bi_size = bio->bi_iter.bi_size - len,
.bi_rw = bio->bi_rw,
};
@@ -800,23 +804,25 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
- bio->bi_iter.bi_size += len;
[PATCH V4] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum
number of segments has been reached, regardless of the fact the page we
are going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation
conditions, a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page only in case
the latter starts a new segment and the maximum number of segments has
already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing `/dev/st0': Device or resource busy
V2: restore the correct number of segments in case of failure.
V3: In case of error, V2 restored the previous number of segments but left
the BIO_SEG_VALID flag set.
To avoid problems, after the page is removed from the bio vec,
V3 performs a recount of the segments in the error code path.
[ming@canonical.com: update bi_iter.bi_size before recounting segments]
V4: merge_bvec_fn() must be called with the old bi_iter.bi_size value.
Signed-off-by: Maurizio Lombardi
Signed-off-by: Ming Lei
---
block/bio.c | 54 ++
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 3e6331d..535b16d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -745,6 +745,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
}
}
+ bio->bi_iter.bi_size += len;
goto done;
}
@@ -761,29 +762,32 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bio->bi_phys_segments++;
+ bio->bi_iter.bi_size += len;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -792,7 +796,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
struct bvec_merge_data bvm = {
.bi_bdev = bio->bi_bdev,
.bi_sector = bio->bi_iter.bi_sector,
- .bi_size = bio->bi_iter.bi_size,
+ .bi_size = bio->bi_iter.bi_size - len,
.bi_rw = bio->bi_rw,
};
@@ -800,23 +804,25 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
- bio->bi_iter.bi_size += len;
Re: [PATCH] bio: merge_bvec_fn() must be called with the old bi_iter.bi_size value
Hi Jens,
On 07/17/2014 10:49 AM, Maurizio Lombardi wrote:
> The patch "bio: modify __bio_add_page() to accept pages that
> don't start a new segment" updates bio->bi_iter.bi_size before
> calling merge_bvec_fn().
>
> This panics the kernel because merge_bvec_fn() expects bi_size to have
> the old value.
>
> This can be reproduced by trying to create a crypto device with cryptsetup.
Can we give to this patch "bio: modify __bio_add_page() to accept pages that
don't start a new segment"
another chance to get in?
Should I squash it with the following fix and resubmit as a single patch or
it's not necessary?
>
> Reported-by: Valdis Kletnieks
> Signed-off-by: Maurizio Lombardi
> ---
> block/bio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index fb12df9..40c5b12 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -795,7 +795,7 @@ static int __bio_add_page(struct request_queue *q, struct
> bio *bio, struct page
> struct bvec_merge_data bvm = {
> .bi_bdev = bio->bi_bdev,
> .bi_sector = bio->bi_iter.bi_sector,
> - .bi_size = bio->bi_iter.bi_size,
> + .bi_size = bio->bi_iter.bi_size - len,
> .bi_rw = bio->bi_rw,
> };
>
>
Thanks,
Maurizio Lombardi
--
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: Bug_ON with patch: bio: modify __bio_add_page() to accept pages that don't start a new segment
Hi Jens, On 07/16/2014 09:53 AM, Jens Axboe wrote: > > Sure, we can try again, hopefully this will be the last of them. > I sent it, it must be applied on top of "bio: modify __bio_add_page() to accept pages that don't start a new segment" http://marc.info/?l=linux-kernel&m=140558697215009&w=2 I really hope this is the last fix to it. Regards, Maurizio Lombardi -- 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/
[PATCH] bio: merge_bvec_fn() must be called with the old bi_iter.bi_size value
The patch "bio: modify __bio_add_page() to accept pages that
don't start a new segment" updates bio->bi_iter.bi_size before
calling merge_bvec_fn().
This panics the kernel because merge_bvec_fn() expects bi_size to have
the old value.
This can be reproduced by trying to create a crypto device with cryptsetup.
[ 25.929846] [ cut here ]
[ 25.929873] kernel BUG at fs/direct-io.c:747!
[ 25.929893] invalid opcode: [#1] PREEMPT SMP
[ 25.929922] Modules linked in:
[ 25.929940] CPU: 3 PID: 308 Comm: systemd-cryptse Not tainted
3.16.0-rc4-next-20140707 #247
[ 25.929974] Hardware name: Dell Inc. Latitude E6530/07Y85M, BIOS A14
01/13/2014
[ 25.930004] task: 880222609e50 ti: 8802225b4000 task.ti:
8802225b4000
[ 25.930034] RIP: 0010:[] []
dio_send_cur_page+0xd7/0xe3
[ 25.930074] RSP: 0018:8802225b7aa0 EFLAGS: 00010202
[ 25.930096] RAX: 0001 RBX: 8802225b7c01 RCX:
[ 25.930126] RDX: RSI: 0001 RDI: 81d13cf0
[ 25.930155] RBP: 8802225b7ac8 R08: R09: 0001
[ 25.930184] R10: R11: R12: 8800c6e8dc00
[ 25.930213] R13: 8802225b7bc0 R14: 007a R15: 007c
[ 25.930243] FS: 7f5908c49840() GS:88022dd8()
knlGS:
[ 25.930276] CS: 0010 DS: ES: CR0: 80050033
[ 25.930300] CR2: 006ecf18 CR3: 000222bb1000 CR4: 001407e0
[ 25.930329] Stack:
[ 25.930339] 0001 8800c6e8dc00 ea0002ba9d68
0800
[ 25.930380] 8802225b7c28 8802225b7b08 8116bfa2
22110780
[ 25.930419] 8800c6e8dc00 ea0002ba9d68 0800
0001
[ 25.930458] Call Trace:
[ 25.930473] [] submit_page_section+0xb1/0x114
[ 25.930499] [] do_blockdev_direct_IO+0xa28/0xd1f
[ 25.930527] [] ? I_BDEV+0xd/0xd
[ 25.930549] [] __blockdev_direct_IO+0x2f/0x31
[ 25.930575] [] ? __blockdev_direct_IO+0x2f/0x31
[ 25.930601] [] ? I_BDEV+0xd/0xd
[ 25.930622] [] blkdev_direct_IO+0x2e/0x30
[ 25.930647] [] ? I_BDEV+0xd/0xd
[ 25.930669] [] generic_file_read_iter+0x93/0x5c8
[ 25.930697] [] blkdev_read_iter+0x35/0x37
[ 25.930722] [] new_sync_read+0x74/0x98
[ 25.930746] [] vfs_read+0xce/0x124
[ 25.930768] [] SyS_read+0x4b/0x79
[ 25.930791] [] system_call_fastpath+0x16/0x1b
[ 25.930816] Code: fe ff ff 48 89 df e8 40 fe ff ff 48 c7 c7 f0 3c d1 81 85
c0 89 45 dc 0f 95 c3 31 d2 0f b6 f3 e8 4f ad f6 ff 84 db 8b 45 dc 74 02 <0f> 0b
5a 5b 41 5c 41 5d 41 5e 5d c3 55 48 89 e5 41 57 4d 89 cf
[ 25.931060] RIP [] dio_send_cur_page+0xd7/0xe3
[ 25.931088] RSP
[ 25.931132] ---[ end trace 5bdcfa6254e32464 ]---
Reported-by: Valdis Kletnieks
Signed-off-by: Maurizio Lombardi
---
block/bio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/bio.c b/block/bio.c
index fb12df9..40c5b12 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -795,7 +795,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
struct bvec_merge_data bvm = {
.bi_bdev = bio->bi_bdev,
.bi_sector = bio->bi_iter.bi_sector,
- .bi_size = bio->bi_iter.bi_size,
+ .bi_size = bio->bi_iter.bi_size - len,
.bi_rw = bio->bi_rw,
};
--
Maurizio Lombardi
--
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: Bug_ON with patch: bio: modify __bio_add_page() to accept pages that don't start a new segment
Hi,
On 07/15/2014 10:44 AM, Maurizio Lombardi wrote:
>> I have reverted it yesterday in my tree.
>>
>
>
> The problem was here:
>
> if (q->merge_bvec_fn) {
> struct bvec_merge_data bvm = {
> .bi_bdev = bio->bi_bdev,
> .bi_sector = bio->bi_iter.bi_sector,
> .bi_size = bio->bi_iter.bi_size, <---
> .bi_rw = bio->bi_rw,
> };
>
> /*
> * merge_bvec_fn() returns number of bytes it can accept
> * at this offset
> */
> if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
> goto failed;
> }
>
> /* If we may be able to merge these biovecs, force a recount */
> if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
> bio->bi_flags &= ~(1 << BIO_SEG_VALID);
>
>
> it should have been ".bi_size = bio->bi_iter.bi_size - len"
>
Jens, will you restore the patch in your tree if I submit
this fix?
Thanks,
Maurizio Lombardi
--
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: Bug_ON with patch: bio: modify __bio_add_page() to accept pages that don't start a new segment
On 07/15/2014 01:38 PM, Maurizio Lombardi wrote: > > > On 07/15/2014 10:14 AM, Mike Qiu wrote: >> My Power7 box boot fail with commit: >> >> 254c4407cb84a6dec90336054615b0f0e996bb7c >> bio: modify __bio_add_page() to accept pages that don't start a new segment >> >> Just revert it will works for me. > > This looks strange to me because, even after reverting my patch, the kernel > still panics > with a different call trace: Never mind, this is another bug in linux-next that is hit by cryptsetup. Sorry for the noise. -- 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: Bug_ON with patch: bio: modify __bio_add_page() to accept pages that don't start a new segment
On 07/15/2014 10:14 AM, Mike Qiu wrote: > My Power7 box boot fail with commit: > > 254c4407cb84a6dec90336054615b0f0e996bb7c > bio: modify __bio_add_page() to accept pages that don't start a new segment > > Just revert it will works for me. This looks strange to me because, even after reverting my patch, the kernel still panics with a different call trace: [ 68.999477] BUG: unable to handle kernel NULL pointer dereference at 0028 [ 69.007411] IP: [] blk_throtl_drain+0x28/0x110 [ 69.013510] PGD 222e5f067 PUD 21bdab067 PMD 0 [ 69.018051] Oops: [#1] SMP [ 69.021335] Modules linked in: serpent_avx_x86_64 serpent_sse2_x86_64 serpent_generic dm_crypt loop iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi cfg80211 rfkill x86_pkg_temp_thermal coretemp kvm_intel bnx2x kvm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel e1000e tpm_infineon ptp iTCO_wdt pcspkr iTCO_vendor_support tpm_tis pps_core microcode ipmi_si serio_raw i2c_i801 ipmi_msghandler mdio tpm ie31200_edac video lpc_ich mfd_core shpchp edac_core xfs libcrc32c ast i2c_algo_bit drm_kms_helper ttm drm ata_generic pata_acpi [ 69.070767] CPU: 3 PID: 11130 Comm: cryptsetup Not tainted 3.16.0-rc5-next-20140714+ #2 [ 69.078862] Hardware name: wortmann To be filled by O.E.M./P8B-M Series, BIOS 6103 12/06/2012 [ 69.087413] task: 8802225c0930 ti: 88021bda task.ti: 88021bda [ 69.094977] RIP: 0010:[] [] blk_throtl_drain+0x28/0x110 [ 69.103529] RSP: 0018:88021bda3b60 EFLAGS: 00010046 [ 69.108866] RAX: RBX: 880222001638 RCX: 7fff [ 69.116110] RDX: 000b RSI: RDI: [ 69.123337] RBP: 88021bda3b78 R08: R09: 8800c9011ff0 [ 69.130562] R10: 88021bda3b78 R11: 8132fd61 R12: 880222001638 [ 69.137764] R13: 88021c84e600 R14: 880222001c30 R15: [ 69.144981] FS: 7f81fd8fb840() GS:88022fd8() knlGS: [ 69.153144] CS: 0010 DS: ES: CR0: 80050033 [ 69.158931] CR2: 0028 CR3: 00021bdac000 CR4: 000407e0 [ 69.166131] Stack: [ 69.168160] 880222001638 880222001c40 88021bda3b88 [ 69.175671] 8131f36e 88021bda3bb8 8130294e 880222001638 [ 69.183182] 819c9580 880222001638 88021af91800 88021bda3bd0 [ 69.190740] Call Trace: [ 69.193218] [] blkcg_drain_queue+0xe/0x10 [ 69.198921] [] __blk_drain_queue+0x6e/0x150 [ 69.204804] [] blk_queue_bypass_start+0x5d/0x80 [ 69.211050] [] blkcg_deactivate_policy+0x38/0x120 [ 69.217437] [] blk_throtl_exit+0x34/0x50 [ 69.223094] [] blkcg_exit_queue+0x3a/0x40 [ 69.228822] [] blk_release_queue+0x26/0xe0 [ 69.234600] [] kobject_cleanup+0x77/0x1b0 [ 69.240345] [] kobject_put+0x28/0x60 [ 69.245596] [] blk_cleanup_queue+0xeb/0x120 [ 69.251464] [] __dm_destroy+0x1d1/0x250 [ 69.257043] [] dm_destroy+0x13/0x20 [ 69.262265] [] dev_remove+0x11e/0x180 [ 69.267614] [] ? dev_suspend+0x250/0x250 [ 69.273269] [] ctl_ioctl+0x246/0x4e0 [ 69.278549] [] dm_ctl_ioctl+0x13/0x20 [ 69.283920] [] do_vfs_ioctl+0x2d8/0x4b0 [ 69.289458] [] SyS_ioctl+0x81/0xa0 [ 69.294544] [] ? __audit_syscall_exit+0x236/0x2e0 [ 69.300964] [] system_call_fastpath+0x16/0x1b [ 69.307020] Code: 00 00 00 66 66 66 66 90 55 48 89 e5 41 55 41 54 49 89 fc 53 4c 8b af e0 06 00 00 49 8b 85 88 00 00 00 31 ff 48 8b 80 68 05 00 00 <48> 8b 70 28 e8 3f 44 de ff 48 85 c0 48 89 c3 74 61 0f 1f 80 00 [ 69.327190] RIP [] blk_throtl_drain+0x28/0x110 [ 69.333402] RSP [ 69.336937] CR2: 0028 [ 69.340275] ---[ end trace 3c89b44a6f5a2b9f ]--- Regards, Maurizio Lombardi -- 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: Bug_ON with patch: bio: modify __bio_add_page() to accept pages that don't start a new segment
On 07/15/2014 10:41 AM, Jens Axboe wrote:
> On 15/07/2014, at 10.14, Mike Qiu wrote:
>>
>> My Power7 box boot fail with commit:
>>
>> 254c4407cb84a6dec90336054615b0f0e996bb7c
>> bio: modify __bio_add_page() to accept pages that don't start a new segment
>>
>> Just revert it will works for me.
>
> I have reverted it yesterday in my tree.
>
The problem was here:
if (q->merge_bvec_fn) {
struct bvec_merge_data bvm = {
.bi_bdev = bio->bi_bdev,
.bi_sector = bio->bi_iter.bi_sector,
.bi_size = bio->bi_iter.bi_size, <---
.bi_rw = bio->bi_rw,
};
/*
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
it should have been ".bi_size = bio->bi_iter.bi_size - len"
Regards,
Maurizio Lombardi
--
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: [Merge branch 'for] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
Hi, On 07/08/2014 10:59 AM, Aaron Lu wrote: > The merge 49b3f10e2cf5c1c25d2ce33ab255cff8a8096ce6 seems to have only one > commit: 254c4407cb84a6dec90336054615b0f0e996bb7c, so I added you guys in. > Please take a look if this is a real problem, thanks. > > > [ 1010.593031] sda: unknown partition table > [ 1010.598052] sd 2:0:0:0: [sda] Attached SCSI disk > [ 1012.893125] sd 2:0:0:0: [sda] Synchronizing SCSI cache > [ 1012.895934] BUG: unable to handle kernel NULL pointer dereference at > 0028 > [ 1012.896336] IP: [] blk_throtl_drain+0x30/0x150 I tried to revert my patch (commit 254c4407cb84a6dec90336054615b0f0e996bb7c) but I'm still able to hit the very same kernel panic in linux-next, so I think my patch is not the one to blame. I was able to reproduce the error in a more easier way: just execute the "reboot" command after the system boot is finished. Regards, Maurizio Lombardi -- 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: [Merge branch 'for] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
Hi,
On 07/08/2014 10:59 AM, Aaron Lu wrote:
>
> [ 1010.593031] sda: unknown partition table
> [ 1010.598052] sd 2:0:0:0: [sda] Attached SCSI disk
> [ 1012.893125] sd 2:0:0:0: [sda] Synchronizing SCSI cache
> [ 1012.895934] BUG: unable to handle kernel NULL pointer dereference at
> 0028
> [ 1012.896336] IP: [] blk_throtl_drain+0x30/0x150
Looks like it is crashing here:
void blk_throtl_drain(struct request_queue *q)
__releases(q->queue_lock) __acquires(q->queue_lock)
{
struct throtl_data *td = q->td;
struct blkcg_gq *blkg;
struct cgroup_subsys_state *pos_css;
struct bio *bio;
int rw;
queue_lockdep_assert_held(q);
rcu_read_lock();
/*
* Drain each tg while doing post-order walk on the blkg tree, so
* that all bios are propagated to td->service_queue. It'd be
* better to walk service_queue tree directly but blkg walk is
* easier.
*/
blkg_for_each_descendant_post(blkg, pos_css, td->queue->root_blkg)
<--
tg_drain_bios(&blkg_to_tg(blkg)->service_queue);
#define blkg_for_each_descendant_post(d_blkg, pos_css, p_blkg) \
css_for_each_descendant_post((pos_css), &(p_blkg)->blkcg->css) \
<--
if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css),\
(p_blkg)->q, false)))
The code tries to access to the blkcg pointer (offset 0x0028 of the blkcg_gq
structure);
so the root_blkg pointer is NULL, hence the kernel panic.
So, IMO, what happens is that the root_blkg pointer is set to NULL by the
blkg_destroy_all() function well before
we reach the blk_throtl_drain() function.
void blkcg_exit_queue(struct request_queue *q)
{
spin_lock_irq(q->queue_lock);
blkg_destroy_all(q); < This is the point where the root_blkg
pointer is destroyed (if I understand the code correctly)
spin_unlock_irq(q->queue_lock);
blk_throtl_exit(q); < This is the function which will execute
blk_throtl_drain()
}
Jens, Ming, do you have any idea?
Regards,
Maurizio Lombardi
> [ 1012.896336] PGD 0
> [ 1012.896336] Oops: [#1] SMP
> [ 1012.896336] Modules linked in: sd_mod scsi_debug(-) crct10dif_generic
> crc_t10dif crct10dif_common loop ipmi_watchdog ipmi_msghandler dm_mod fuse sg
> sr_mod cdrom ata_generic pata_acpi parport_pc parport floppy snd_pcm
> snd_timer snd cirrus ata_piix soundcore syscopyarea pcspkr sysfillrect
> sysimgblt ttm drm_kms_helper libata drm i2c_piix4
> [ 1012.896336] CPU: 1 PID: 8020 Comm: rmmod Not tainted
> 3.16.0-rc3-01927-ge376abf #1
> [ 1012.896336] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [ 1012.896336] task: 8801151a ti: 880079668000 task.ti:
> 880079668000
> [ 1012.896336] RIP: 0010:[] []
> blk_throtl_drain+0x30/0x150
> [ 1012.896336] RSP: 0018:88007966bb60 EFLAGS: 00010046
> [ 1012.896336] RAX: RBX: 8800bdbba6e8 RCX:
> 88007dea1a20
> [ 1012.896336] RDX: RSI: RDI:
>
> [ 1012.896336] RBP: 88007966bb78 R08: R09:
> 0046
> [ 1012.896336] R10: 88007966bb78 R11: 0246 R12:
> 8800bdbba6e8
> [ 1012.896336] R13: 880091ba3800 R14: 8800bdbbad40 R15:
> 880030a13120
> [ 1012.896336] FS: 7fa159320700() GS:88011fc8()
> knlGS:
> [ 1012.896336] CS: 0010 DS: ES: CR0: 8005003b
> [ 1012.896336] CR2: 0028 CR3: 7f42e000 CR4:
> 06e0
> [ 1012.896336] Stack:
> [ 1012.896336] 8800bdbba6e8 8800bdbbad50
> 88007966bb88
> [ 1012.896336] 813cc8ce 88007966bbb8 813b1aac
> 8800bdbba6e8
> [ 1012.896336] 81cf9200 8800bdbba6e8 880030a13000
> 88007966bbd0
> [ 1012.896336] Call Trace:
> [ 1012.896336] [] blkcg_drain_queue+0xe/0x10
> [ 1012.896336] [] __blk_drain_queue+0x7c/0x180
> [ 1012.896336] [] blk_queue_bypass_start+0x8e/0xd0
> [ 1012.896336] [] blkcg_deactivate_policy+0x38/0x140
> [ 1012.896336] [] blk_throtl_exit+0x34/0x50
> [ 1012.896336] [] blkcg_exit_queue+0x48/0x70
> [ 1012.896336] [] blk_release_queue+0x26/0x100
> [ 1012.896336] [] kobject_cleanup+0x77/0x1b0
> [ 1012.896336] [] kobject_put+0x28/0x60
> [ 1012.896336] [] blk_put_queue+0x15/0x20
> [ 1012.896336] []
> scsi_device_dev_release_usercontext+0xbb/0x120
> [ 1012.896336] [] execute_in_process_context+0x67/0x70
> [ 1012.896336] [] scsi_device_dev_release+0x1c/0x20
> [ 1012.896336] [] device_release+0x32/0xa0
> [ 1012.896336] [] kobject_cleanup+0x77/0x1b0
>
Re: [PATCH] block/bio.c: update bi_iter.bi_size before recounting segments
Hi, I don't see this patch in linux-next yet nor a review. Jens, Andrew; did you notice it? On 05/29/2014 09:59 AM, Ming Lei wrote: > The patch of "bio: modify __bio_add_page() to accept pages that > don't start a new segment" changes the way for adding one page > to bio: > > - previously by adding page after checking successfully > - now by trying to add page and recover if it fails > > Unfortunately the patch forgets to update bio->bi_iter.bi_size > before trying to add page, then the last vector for holding > the added page may not be covered if recouning segments is needed, > so bio->bi_phys_segments may become not consistent with the > actual bio page buffers after the page is added successfully > to the bio(after bi_iter.bi_size is added by 'len') > > Suppose the page in the last vector can't be merged to bio, tragedy > will happen when __bio_add_page() is called to add another page: > > - blk_recount_segments() is called and the actual segments get > figured out correctly > > - the actual segments may become queue_max_segments(q) plus one > in failure path > > - driver will find the segment count is too big to handle. > > The patch fixes the virtio-blk oops bug reported from Jet Chen in > below link: > > http://marc.info/?l=linux-kernel&m=140113053817095&w=2 > > Cc: Jens Axboe > Cc: Maurizio Lombardi > Cc: Dongsu Park > Cc: Christoph Hellwig > Cc: Kent Overstreet > Cc: Andrew Morton > Reported-by: Jet Chen > Tested-by: Jet Chen > Signed-off-by: Ming Lei > --- > Andrew, could you put the patch in your -mm tree > because the previous two patches were routed from > your tree? > > block/bio.c |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/bio.c b/block/bio.c > index 0443694..f9bae56 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -744,6 +744,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > } > } > > + bio->bi_iter.bi_size += len; > goto done; > } > } > @@ -761,6 +762,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_offset = offset; > bio->bi_vcnt++; > bio->bi_phys_segments++; > + bio->bi_iter.bi_size += len; > > /* >* Perform a recount if the number of segments is greater > @@ -802,7 +804,6 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bio->bi_flags &= ~(1 << BIO_SEG_VALID); > > done: > - bio->bi_iter.bi_size += len; > return len; > > failed: > @@ -810,6 +811,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_len = 0; > bvec->bv_offset = 0; > bio->bi_vcnt--; > + bio->bi_iter.bi_size -= len; > blk_recount_segments(q, bio); > return 0; > } > -- 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 v2] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
On 06/26/2014 01:54 PM, Rickard Strandqvist wrote: > A struct member variable is set to different values without having used in > between. It is almost ok for me but I think you should mention that it also fixes a bug, or the commit message will be misleading. > > This was found using a static code analysis program called cppcheck > > Signed-off-by: Rickard Strandqvist > --- > drivers/scsi/bnx2i/bnx2i_iscsi.c |3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c > b/drivers/scsi/bnx2i/bnx2i_iscsi.c > index 166543f..4e17a7f 100644 > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c > @@ -1643,12 +1643,11 @@ static void bnx2i_conn_get_stats(struct > iscsi_cls_conn *cls_conn, > stats->r2t_pdus = conn->r2t_pdus_cnt; > stats->tmfcmd_pdus = conn->tmfcmd_pdus_cnt; > stats->tmfrsp_pdus = conn->tmfrsp_pdus_cnt; > - stats->custom_length = 3; > strcpy(stats->custom[2].desc, "eh_abort_cnt"); > stats->custom[2].value = conn->eh_abort_cnt; > stats->digest_err = 0; > stats->timeout_err = 0; > - stats->custom_length = 0; > + stats->custom_length = 3; > } > > > -- 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 v2] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
On 06/26/2014 02:05 PM, Joe Perches wrote: > On Thu, 2014-06-26 at 13:54 +0200, Rickard Strandqvist wrote: >> A struct member variable is set to different values without having used in >> between. > [] >> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c >> b/drivers/scsi/bnx2i/bnx2i_iscsi.c > [] >> @@ -1643,12 +1643,11 @@ static void bnx2i_conn_get_stats(struct >> iscsi_cls_conn *cls_conn, >> stats->r2t_pdus = conn->r2t_pdus_cnt; >> stats->tmfcmd_pdus = conn->tmfcmd_pdus_cnt; >> stats->tmfrsp_pdus = conn->tmfrsp_pdus_cnt; >> -stats->custom_length = 3; >> strcpy(stats->custom[2].desc, "eh_abort_cnt"); >> stats->custom[2].value = conn->eh_abort_cnt; >> stats->digest_err = 0; >> stats->timeout_err = 0; >> -stats->custom_length = 0; >> +stats->custom_length = 3; > > You are changing custom_length from 0 to 3. > > Why is this correct? http://marc.info/?l=linux-scsi&m=140371670511706&w=2 -- 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] scsi: pm8001: pm80xx_hwi.c: Cleaning up variable is set more than once
On 06/26/2014 10:09 AM, Jack Wang wrote: > Thanks Rickard, > > From my point of view, looks good, but I'd like to get review from Anand > (cc-ed). I would like to add that I noticed that this fields is only set and appears to be never used, maybe it could be completely removed. Regards, Maurizio Lombardi -- 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] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
Hi, On 06/26/2014 02:28 AM, Rickard Strandqvist wrote: > > If it's not obvious, I do all my fixes without changing the previous > intent. But obviously it is not always right, rather it is one of main > reasons to fix this type of error :) Yes it is obvious, your patch was correct (it didn't modify the function behaviour) but helped to spot a defect. > > But I'll make a new patch then, with = 3 ? Yes, please submit a new patch which sets custom_length = 3 at the end of the function. Thanks, Maurizio Lombardi -- 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] scsi: pm8001: pm80xx_hwi.c: Cleaning up variable is set more than once
Hi, On 06/25/2014 05:41 PM, Purush Gupta wrote: > Its possible HW may require programming those fields? I'm looking at the code and it doesn't look so, did you see something suspicious? > May be original > contributor of the driver should review...No offense! I believe it requires the maintainer ACK and it takes precedence in any case, am I wrong? Regards, Maurizio Lombardi > > thanks, > Purush > > > On Wed, Jun 25, 2014 at 8:34 AM, Maurizio Lombardi > wrote: > >> This one looks good to me, >> >> Reviewed-by: Maurizio Lombardi >> >> On 06/25/2014 04:01 PM, Rickard Strandqvist wrote: >>> A struct member variable is set to different values without having used >> in between. >>> >>> This was found using a static code analysis program called cppcheck >>> >>> Signed-off-by: Rickard Strandqvist < >> rickard_strandqv...@spectrumdigital.se> >>> --- >>> drivers/scsi/pm8001/pm80xx_hwi.c |1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c >> b/drivers/scsi/pm8001/pm80xx_hwi.c >>> index d70587f..2698227 100644 >>> --- a/drivers/scsi/pm8001/pm80xx_hwi.c >>> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c >>> @@ -249,7 +249,6 @@ moreData: >>> sprintf(pm8001_ha-> >>> forensic_info.data_buf.direct_data, >>> "%08x ", 4); >>> - pm8001_ha->forensic_info.data_buf.read_len = >> 0x; >>> pm8001_ha->forensic_info.data_buf.direct_len = 0; >>> pm8001_ha->forensic_info.data_buf.direct_offset = >> 0; >>> pm8001_ha->forensic_info.data_buf.read_len = 0; >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- 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] scsi: pm8001: pm80xx_hwi.c: Cleaning up variable is set more than once
This one looks good to me, Reviewed-by: Maurizio Lombardi On 06/25/2014 04:01 PM, Rickard Strandqvist wrote: > A struct member variable is set to different values without having used in > between. > > This was found using a static code analysis program called cppcheck > > Signed-off-by: Rickard Strandqvist > --- > drivers/scsi/pm8001/pm80xx_hwi.c |1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c > b/drivers/scsi/pm8001/pm80xx_hwi.c > index d70587f..2698227 100644 > --- a/drivers/scsi/pm8001/pm80xx_hwi.c > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c > @@ -249,7 +249,6 @@ moreData: > sprintf(pm8001_ha-> > forensic_info.data_buf.direct_data, > "%08x ", 4); > - pm8001_ha->forensic_info.data_buf.read_len = 0x; > pm8001_ha->forensic_info.data_buf.direct_len = 0; > pm8001_ha->forensic_info.data_buf.direct_offset = 0; > pm8001_ha->forensic_info.data_buf.read_len = 0; > -- 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] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
Hi, On 06/25/2014 04:04 PM, Rickard Strandqvist wrote: > A struct member variable is set to different values without having used in > between. > > This was found using a static code analysis program called cppcheck > > Signed-off-by: Rickard Strandqvist > --- > drivers/scsi/bnx2i/bnx2i_iscsi.c |1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c > b/drivers/scsi/bnx2i/bnx2i_iscsi.c > index 166543f..fdf7bc3 100644 > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c > @@ -1643,7 +1643,6 @@ static void bnx2i_conn_get_stats(struct iscsi_cls_conn > *cls_conn, > stats->r2t_pdus = conn->r2t_pdus_cnt; > stats->tmfcmd_pdus = conn->tmfcmd_pdus_cnt; > stats->tmfrsp_pdus = conn->tmfrsp_pdus_cnt; > - stats->custom_length = 3; > strcpy(stats->custom[2].desc, "eh_abort_cnt"); > stats->custom[2].value = conn->eh_abort_cnt; > stats->digest_err = 0; > Eddie, The code modifies the content of stats->custom[2], so shouldn't custom_length be set to 3? Why is it set to zero at the end of this function? Regards, Maurizio Lombardi -- 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] bio: decrease bi_iter.bi_size by len in the fail path
On Thu, May 29, 2014 at 02:06:18PM +0800, Jet Chen wrote: > This patch works, thanks. > > Tested-by: Jet Chen > > diff --git a/block/bio.c b/block/bio.c > index 0443694..f9bae56 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -744,6 +744,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > } > } > > + bio->bi_iter.bi_size += len; > goto done; > } > } > @@ -761,6 +762,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_offset = offset; > bio->bi_vcnt++; > bio->bi_phys_segments++; > + bio->bi_iter.bi_size += len; > > /* >* Perform a recount if the number of segments is greater > @@ -802,7 +804,6 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bio->bi_flags &= ~(1 << BIO_SEG_VALID); > > done: > - bio->bi_iter.bi_size += len; > return len; > > failed: > @@ -810,6 +811,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_len = 0; > bvec->bv_offset = 0; > bio->bi_vcnt--; > + bio->bi_iter.bi_size -= len; > blk_recount_segments(q, bio); > return 0; > } Good! Jens, can you review and merge it? Thanks, Maurizio Lombardi -- 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] bio: decrease bi_iter.bi_size by len in the fail path
Hi Ming, On Thu, May 29, 2014 at 12:59:19AM +0800, Ming Lei wrote: > > Actually, the correct thing may be like what did in the > attached patch, as Maurizio discussed with me[1]. > > Very interestingly, I have reproduced the problem one time > with ext4/271 ext4/301 ext4/305, but won't with the attached > patch after running it for 3 rounds. > > [tom@localhost xfstests]$ sudo ./check ext4/271 ext4/301 ext4/305 > FSTYP -- ext4 > PLATFORM -- Linux/x86_64 localhost 3.15.0-rc7-next-20140527+ > MKFS_OPTIONS -- /dev/vdc > MOUNT_OPTIONS -- -o acl,user_xattr /dev/vdc /mnt/scratch > > ext4/271 1s ... 1s > ext4/301 31s ... 32s > ext4/305 181s ... 180s > Ran: ext4/271 ext4/301 ext4/305 > Passed all 3 tests > > Jet, could you test the attached patch? > > [1], https://lkml.org/lkml/2014/5/27/327 There is a little mistake in your patch, you removed bio->bi_iter.bi_size += len; after the "done" label, but be careful that at line 747 there is a "goto done"... bi_size should be incremented before jumping there. Thanks, Maurizio Lombardi -- 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] bio: decrease bi_iter.bi_size by len in the fail path
Hi,
thanks for the patch
On Wed, May 28, 2014 at 05:09:38PM +0200, Dongsu Park wrote:
> From: Dongsu Park
>
> Commit 3979ef4dcf3d1de55a560a3a4016c30a835df44d ("bio-modify-
> __bio_add_page-to-accept-pages-that-dont-start-a-new-segment-v3")
> introduced a regression as reported by Jet Chen.
> That results in a kernel BUG at drivers/block/virtio_blk.c:166.
>
> To fix that, bi_iter.bi_size must be decreased by len, before
> recounting the number of physical segments.
I don't understand exactly why it must be decreased by len.
Can you provide more info?
Thanks,
Maurizio Lombardi
>
> Tested on with kernel 3.15.0-rc7-next-20140527 on qemu guest,
> by running xfstests/ext4/271.
>
> Cc: Jens Axboe
> Cc: Jet Chen
> Cc: Maurizio Lombardi
> Signed-off-by: Dongsu Park
> ---
> block/bio.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/block/bio.c b/block/bio.c
> index 0443694ccbb4..67d7cba1e5fd 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -810,6 +810,7 @@ static int __bio_add_page(struct request_queue *q, struct
> bio *bio, struct page
> bvec->bv_len = 0;
> bvec->bv_offset = 0;
> bio->bi_vcnt--;
> + bio->bi_iter.bi_size -= len;
> blk_recount_segments(q, bio);
> return 0;
> }
> --
> 1.9.1
>
--
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: [jet.c...@intel.com: [bio] kernel BUG at drivers/block/virtio_blk.c:166!]
Hi, On Wed, May 28, 2014 at 11:16:17PM +0800, Jet Chen wrote: > > Sorry for late respond. Dongsu has sent a patch for this issue. > message-id > <1401289778-9840-1-git-send-email-dongsu.p...@profitbricks.com> > Do you still need me to test the following patch ? No, don't test my patch, it is not needed anymore, please test Dongsu's patch instead so we are sure the regression is fixed. Thanks, Maurizio Lombardi -- 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 V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
On Tue, May 27, 2014 at 10:04:58PM +0800, Ming Lei wrote: > > Looks your approach is simpler. > > But looks there is one problem if I understand correctly: > __blk_recalc_rq_segments() may not cover the last vector > because bio->bi_iter.bi_size isn't updated until the end of > __bio_add_page(). > > But it shouldn't have been related with current virtio-blk problem. > This is a valid point, bi_iter.bi_size influences the behaviour of blk_recount_segments(). Maybe Jens can confirm your observation. Anyway it doesn't explain the reason behind the regression introduced by commit 3979ef4dcf Maurizio Lombardi -- 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 V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
On Tue, May 27, 2014 at 06:15:45PM +0800, Ming Lei wrote: > > If the page can be merged to last segment, it should have been > covered by code in branch of 'if (bio->bi_vcnt > 0) ...', shouldn't it? > > Or maybe it is better to make that code cover your case since > looks your case is similar with that one according to your commit > log. > I realized that maybe you mean this branch: if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec))) right? In this case the branch is not even reached in the original code because the function returned an error way before executing it (line 760) There was already a patchset trying to modify the code in a different way than mine: https://groups.google.com/forum/#!msg/linux.kernel/3IanUpBVhFQ/3Xbg3yLRFp4J but it has been ignored and in my opinion it takes a more complicated approach. Regards, Maurizio Lombardi -- 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: [jet.c...@intel.com: [bio] kernel BUG at drivers/block/virtio_blk.c:166!]
On Tue, May 27, 2014 at 10:43:59AM +0200, Maurizio Lombardi wrote:
>
> But now I'm suspicious of this part of commit 3979ef4dcf:
>
> failed:
> bvec->bv_page = NULL;
> bvec->bv_len = 0;
> bvec->bv_offset = 0;
> bio->bi_vcnt--; <
> blk_recount_segments(q, bio);
> return 0;
>
> Is decreasing bi_vcnt sufficient to guarantee that blk_recount_segments()
> recalculates the correct number of physical segments?
> Looking at the __blk_recalc_rq_segments() it appears it may not be the case.
>
> The question is how can we restore the correct number of physical segments in
> case
> of failure without breaking anything...
>
If my hypothesis is correct, the following patch should trigger a kernel panic,
Jet Chen, can you try it and let me know whether the BUG_ON is hit or not?
diff --git a/block/bio.c b/block/bio.c
index 0443694..763868f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -701,6 +701,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
unsigned int max_sectors)
{
int retried_segments = 0;
+ unsigned int phys_segments_orig;
struct bio_vec *bvec;
/*
@@ -751,6 +752,9 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
if (bio->bi_vcnt >= bio->bi_max_vecs)
return 0;
+ blk_recount_segments(q, bio);
+ phys_segments_orig = bio->bi_phys_segments;
+
/*
* setup the new entry, we might clear it again later if we
* cannot add the page
@@ -811,6 +815,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
bvec->bv_offset = 0;
bio->bi_vcnt--;
blk_recount_segments(q, bio);
+ BUG_ON(phys_segments_orig != bio->bi_phys_segments);
return 0;
}
Regards,
Maurizio Lombardi
--
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 V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
On Tue, May 27, 2014 at 06:15:45PM +0800, Ming Lei wrote: > > If the page can be merged to last segment, it should have been > covered by code in branch of 'if (bio->bi_vcnt > 0) ...', shouldn't it? > > Or maybe it is better to make that code cover your case since > looks your case is similar with that one according to your commit > log. the code in this branch does not cover our case, it is intended to cover the case where __bio_add_page() is called multiple times with the *same* page as parameter. My patch deals with the case when __bio_add_page() is called with *different* pages as parameter but physically adjacent to each other. That said it is true that maybe this branch can be extended to also cover the case I'm dealing with and try to avoid the problem that commit 3979ef4dcf introduced. Thanks, Maurizio Lombardi -- 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: [jet.c...@intel.com: [bio] kernel BUG at drivers/block/virtio_blk.c:166!]
On Tue, May 27, 2014 at 12:03:29PM +0800, Ming Lei wrote:
> On Tue, May 27, 2014 at 3:43 AM, Maurizio Lombardi
> wrote:
> > Hi Jens,
> >
> > looks like that commit 3979ef4dcf3d1de55a560a3a4016c30a835df44d
> > ("bio-modify-__bio_add_page-to-accept-pages-that-dont-start-a-new-segment-v3")
> > introduces a regression, as reported by Jet Chan.
> >
> > Do you have any idea about the possible problem with this patch?
> >
> > it is the one that performs a recount of the segments in case of failure in
> > __bio_add_page()
> >
> > http://www.spinics.net/lists/mm-commits/msg103684.html
> >
> > I would not be surprised if the bug was introduced by fceb38f36f, because it
> > contained a mystake that commit 3979ef4dcf supposedly fixed.
> > But learning that commit 3979ef4dcf is introducing a regression leaves
> > me quite puzzled.
>
> From code of __blk_recalc_rq_segments(), looks it
> won't check if recounted physical segment number is
> bigger than queue_max_segments(), so wondering if
> blk_recount_segments() can always decrease
> physical segment number.
>
This is what __bio_add_page() did before both fceb38f36f and 3979ef4dcf at line
757
while (bio->bi_phys_segments >= queue_max_segments(q)) {
if (retried_segments)
return 0;
retried_segments = 1;
blk_recount_segments(q, bio);
}
so it is possible, in case of error, to return from the function even if the
recounted
physical segments are bigger than queue_max_segments(q).
-
But now I'm suspicious of this part of commit 3979ef4dcf:
failed:
bvec->bv_page = NULL;
bvec->bv_len = 0;
bvec->bv_offset = 0;
bio->bi_vcnt--; <
blk_recount_segments(q, bio);
return 0;
Is decreasing bi_vcnt sufficient to guarantee that blk_recount_segments()
recalculates the correct number of physical segments?
Looking at the __blk_recalc_rq_segments() it appears it may not be the case.
The question is how can we restore the correct number of physical segments in
case
of failure without breaking anything...
Regards,
Maurizio Lombardi
--
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/
[PATCH V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum number
of segments has been reached, regardless of the fact the page we are
going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation conditions,
a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page
only in case the latter starts a new segment and the maximum number
of segments has already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing ‘/dev/st0’: Device or resource busy
Changes in V3:
In case of error, V2 restored the previous number of segments but left
the BIO_SEG_FLAG set.
To avoid problems, after the page is removed from the bio vec,
V3 performs a recount of the segments in the error code path.
Signed-off-by: Maurizio Lombardi
---
fs/bio.c | 48 ++--
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/fs/bio.c b/fs/bio.c
index 6f0362b..9bf512e 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -750,29 +750,31 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bio->bi_phys_segments++;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -789,23 +791,25 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
bio->bi_iter.bi_size += len;
return len;
+
+ failed:
+ bvec->bv_page = NULL;
+ bvec->bv_len = 0;
+ bvec->bv_offset = 0;
+ bio->bi_vcnt--;
+ blk_recount_segments(q, bio);
+ return 0;
}
/**
--
Maurizio Lombardi
--
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] bio: modify __bio_add_page() to accept pages that don't start a new segment
Sorry I did a mistake in this patch: on failure I should restore the original
value
of bi_phys_segments.
I'm going to send a new version.
Maurizio Lombardi
On Tue, Apr 29, 2014 at 04:58:18PM +0200, Maurizio Lombardi wrote:
> The original behaviour is to refuse to add a new page if the maximum number
> of segments has been reached, regardless of the fact the page we are
> going to add can be merged into the last segment or not.
>
> Unfortunately, when the system runs under heavy memory fragmentation
> conditions,
> a driver may try to add multiple pages to the last segment.
> The original code won't accept them and EBUSY will be reported to
> userspace.
>
> This patch modifies the function so it refuses to add a page
> only in case the latter starts a new segment and the maximum number
> of segments has already been reached.
>
> The bug can be easily reproduced with the st driver:
>
> 1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
> 2) modprobe st buffer_kbs=1024
> 3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
>dd: error writing ‘/dev/st0’: Device or resource busy
>
> Signed-off-by: Maurizio Lombardi
> ---
> fs/bio.c | 50 --
> 1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/fs/bio.c b/fs/bio.c
> index 6f0362b..9a3a0b1 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -750,29 +750,31 @@ static int __bio_add_page(struct request_queue *q,
> struct bio *bio, struct page
> return 0;
>
> /*
> - * we might lose a segment or two here, but rather that than
> - * make this too complex.
> + * setup the new entry, we might clear it again later if we
> + * cannot add the page
> + */
> + bvec = &bio->bi_io_vec[bio->bi_vcnt];
> + bvec->bv_page = page;
> + bvec->bv_len = len;
> + bvec->bv_offset = offset;
> + bio->bi_vcnt++;
> + bio->bi_phys_segments++;
> +
> + /*
> + * Perform a recount if the number of segments is greater
> + * than queue_max_segments(q).
>*/
>
> - while (bio->bi_phys_segments >= queue_max_segments(q)) {
> + while (bio->bi_phys_segments > queue_max_segments(q)) {
>
> if (retried_segments)
> - return 0;
> + goto failed;
>
> retried_segments = 1;
> blk_recount_segments(q, bio);
> }
>
> /*
> - * setup the new entry, we might clear it again later if we
> - * cannot add the page
> - */
> - bvec = &bio->bi_io_vec[bio->bi_vcnt];
> - bvec->bv_page = page;
> - bvec->bv_len = len;
> - bvec->bv_offset = offset;
> -
> - /*
>* if queue has other restrictions (eg varying max sector size
>* depending on offset), it can specify a merge_bvec_fn in the
>* queue to get further control
> @@ -789,23 +791,27 @@ static int __bio_add_page(struct request_queue *q,
> struct bio *bio, struct page
>* merge_bvec_fn() returns number of bytes it can accept
>* at this offset
>*/
> - if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
> - bvec->bv_page = NULL;
> - bvec->bv_len = 0;
> - bvec->bv_offset = 0;
> - return 0;
> - }
> + if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
> + goto failed;
> }
>
> /* If we may be able to merge these biovecs, force a recount */
> - if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
> + if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
> bio->bi_flags &= ~(1 << BIO_SEG_VALID);
>
> - bio->bi_vcnt++;
> - bio->bi_phys_segments++;
> done:
> bio->bi_iter.bi_size += len;
> return len;
> +
> + failed:
> + bvec->bv_page = NULL;
> + bvec->bv_len = 0;
> + bvec->bv_offset = 0;
> + bio->bi_vcnt--;
> + if (!retried_segments)
> + bio->bi_phys_segments--;
> +
> + return 0;
> }
>
> /**
> --
> Maurizio Lombardi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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/
[PATCH V2] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum number
of segments has been reached, regardless of the fact the page we are
going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation conditions,
a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page
only in case the latter starts a new segment and the maximum number
of segments has already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing ‘/dev/st0’: Device or resource busy
Signed-off-by: Maurizio Lombardi
---
fs/bio.c | 51 +--
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/fs/bio.c b/fs/bio.c
index 6f0362b..a31e12b 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -699,6 +699,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
unsigned int max_sectors)
{
int retried_segments = 0;
+ unsigned int bi_phys_segments_orig;
struct bio_vec *bvec;
/*
@@ -750,29 +751,32 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bi_phys_segments_orig = bio->bi_phys_segments;
+ bio->bi_phys_segments++;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -789,23 +793,26 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
bio->bi_iter.bi_size += len;
return len;
+
+ failed:
+ bvec->bv_page = NULL;
+ bvec->bv_len = 0;
+ bvec->bv_offset = 0;
+ bio->bi_vcnt--;
+ bio->bi_phys_segments = bi_phys_segments_orig;
+
+ return 0;
}
/**
--
Maurizio Lombardi
--
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/
[PATCH] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum number
of segments has been reached, regardless of the fact the page we are
going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation conditions,
a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page
only in case the latter starts a new segment and the maximum number
of segments has already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing ‘/dev/st0’: Device or resource busy
Signed-off-by: Maurizio Lombardi
---
fs/bio.c | 50 --
1 file changed, 28 insertions(+), 22 deletions(-)
diff --git a/fs/bio.c b/fs/bio.c
index 6f0362b..9a3a0b1 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -750,29 +750,31 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bio->bi_phys_segments++;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -789,23 +791,27 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
bio->bi_iter.bi_size += len;
return len;
+
+ failed:
+ bvec->bv_page = NULL;
+ bvec->bv_len = 0;
+ bvec->bv_offset = 0;
+ bio->bi_vcnt--;
+ if (!retried_segments)
+ bio->bi_phys_segments--;
+
+ return 0;
}
/**
--
Maurizio Lombardi
--
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/
[PATCH] intel_pstate: fix race condition in intel_pstate_init()
There is a race condition in the intel pstate driver initialization:
the cpufreq_register_driver() function creates a timer that makes use of the
energy_divisor variable (intel_pstate_timer_func), the problem is that
energy_divisor is initialized
*after* the call to cpufreq_register_driver().
This patch fixes the bug by initializing energy_divisor before the
call to cpufreq_register_driver().
[2.351047] Intel pstate controlling: cpu 2
[2.355271] divide error: [#1] SMP
[2.359222] Modules linked in:
[2.362291] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.14.0-rc1-linux-mainline+ #7
[2.369933] Hardware name: wortmann To be filled by O.E.M./P8B-M Series,
BIOS 6103 12/06/2012
[2.378441] task: 880223948000 ti: 880223944000 task.ti:
880223944000
[2.385912] RIP: 0010:[] []
intel_pstate_timer_func+0x2b7/0x430
[2.395060] RSP: :880226e03d58 EFLAGS: 00010246
[2.400363] RAX: 06130063 RBX: 88022191b800 RCX: 0199
[2.407486] RDX: RSI: 2000 RDI: 0199
[2.414608] RBP: 880226e03dd8 R08: 88022191b850 R09: 880223948cc8
[2.421731] R10: R11: R12: 0002
[2.428854] R13: 06130063 R14: 003b3a91a54e R15: 880226e03da4
[2.435984] FS: () GS:880226e0()
knlGS:
[2.444060] CS: 0010 DS: ES: CR0: 80050033
[2.449794] CR2: 88022000 CR3: 01a0e000 CR4: 000407f0
[2.456917] Stack:
[2.458927] 880226e03dd8 0246
8108a0c0
[2.466380] 82dc18d0 880226e03d98
0246
[2.473832] 26e03da8 26e03e08 0038
88022191b848
[2.481281] Call Trace:
[2.483725]
[2.485651] [] ? cascade+0xb0/0xb0
[2.490912] [] ?
intel_pstate_platform_pwr_mgmt_exists+0x1e0/0x1e0
[2.498736] [] call_timer_fn+0x98/0x260
[2.504218] [] ? cascade+0xb0/0xb0
[2.509263] [] ? _raw_spin_unlock_irq+0x30/0x50
[2.515439] [] run_timer_softirq+0x298/0x310
[2.521351] [] ? rcu_irq_exit+0x8f/0xe0
[2.526833] [] ? retint_restore_args+0x13/0x13
[2.532919] [] ?
intel_pstate_platform_pwr_mgmt_exists+0x1e0/0x1e0
[2.540741] [] __do_softirq+0x144/0x4f0
[2.546217] [] irq_exit+0x14d/0x180
[2.551346] [] smp_apic_timer_interrupt+0x4a/0x60
[2.557688] [] apic_timer_interrupt+0x72/0x80
[2.563685]
[2.565608] [] ? mark_held_locks+0x90/0x150
[2.571639] [] ? vprintk_emit+0x1c0/0x610
[2.577299] [] ? _raw_spin_unlock_irqrestore+0x40/0x80
[2.584083] [] printk+0x77/0x79
[2.588873] [] ? core_get_turbo_pstate+0x3c/0x60
[2.595139] [] intel_pstate_init_cpu+0x395/0x3a0
[2.601404] [] intel_pstate_cpu_init+0x1b/0x100
[2.607580] [] __cpufreq_add_dev+0x213/0x780
[2.613491] [] ? klist_next+0x8e/0x120
[2.618887] [] cpufreq_add_dev+0x10/0x20
[2.624451] [] subsys_interface_register+0xb1/0xe0
[2.630880] [] ? trace_hardirqs_on+0xd/0x10
[2.636711] [] cpufreq_register_driver+0xfe/0x2a0
[2.643055] [] intel_pstate_init+0x14a/0x310
[2.648972] [] ? mutex_unlock+0xe/0x10
[2.654361] [] ? intel_pstate_setup+0x2e/0x2e
[2.660357] [] do_one_initcall+0xda/0x180
[2.666008] [] ? kernel_init_freeable+0x31a/0x31a
[2.672357] [] do_basic_setup+0x9d/0xc0
[2.677833] [] ? kernel_init_freeable+0x31a/0x31a
[2.684176] [] kernel_init_freeable+0x297/0x31a
[2.690345] [] ? kernel_init+0xe/0xf0
[2.695648] [] ? trace_hardirqs_on_caller+0x105/0x1d0
[2.702339] [] ? rest_init+0x170/0x170
[2.707734] [] kernel_init+0xe/0xf0
[2.712866] [] ret_from_fork+0x7c/0xb0
[2.718262] [] ? rest_init+0x170/0x170
[2.723651] Code: c8 00 00 00 48 89 df 01 d6 e8 b6 f1 ff ff eb 0e 0f 1f 40
00 29 d6 48 89 df e8 a6 f1 ff ff 4b 8d 04 a4 31 d2 4c 8d 04 c3 4c 89 e8 <48> f7
35 fa 8d 9b 01 45 8b b8 40 01 00 00 41 89 c1 49 8b 80 28
[2.743596] RIP [] intel_pstate_timer_func+0x2b7/0x430
[2.750390] RSP
[2.751934] tsc: Refined TSC clocksource calibration: 3092.975 MHz
[2.760048] divide error: [#2] [2.760064] ---[ end trace
b87c91b37801378a ]---
[2.761931] Kernel panic - not syncing: Fatal exception in interrupt
Signed-off-by: Maurizio Lombardi
---
drivers/cpufreq/intel_pstate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 79606f4..bc04e73 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -956,13 +956,13 @@ static int __init intel_pstate_init(void)
if (!all_cpu_data)
return -ENOMEM;
+ rdmsrl(MSR_RAPL_POWER_UNIT, units);
+ energy_divisor = 1 << ((units >> 8) & 0x1f); /* bits{12:8} */
+
rc = c
[PATCH RESEND] wlags49_h2: Fix overflow in wireless_set_essid()
This patch prevents the wireless_set_essid() function from overwriting
the last byte of the NetworkName buffer which must be NULL.
Signed-off-by: Maurizio Lombardi
---
drivers/staging/wlags49_h2/wl_wext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wlags49_h2/wl_wext.c
b/drivers/staging/wlags49_h2/wl_wext.c
index 4a1ddaf..187fc06 100644
--- a/drivers/staging/wlags49_h2/wl_wext.c
+++ b/drivers/staging/wlags49_h2/wl_wext.c
@@ -1061,7 +1061,7 @@ static int wireless_set_essid(struct net_device *dev,
struct iw_request_info *in
goto out;
}
- if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN + 1) {
+ if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN) {
ret = -EINVAL;
goto out;
}
--
Maurizio Lombardi
--
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/
[PATCH v2] wlags49_h2: Fix overflow in wireless_set_essid()
This patch prevents the wireless_set_essid() function from overwriting
the last byte of the NetworkName buffer which must be NULL.
Signed-off-by: Maurizio Lombardi
---
drivers/staging/wlags49_h2/wl_wext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wlags49_h2/wl_wext.c
b/drivers/staging/wlags49_h2/wl_wext.c
index c731ff2..62ad158 100644
--- a/drivers/staging/wlags49_h2/wl_wext.c
+++ b/drivers/staging/wlags49_h2/wl_wext.c
@@ -1127,7 +1127,7 @@ static int wireless_set_essid(struct net_device *dev,
struct iw_request_info *in
goto out;
}
- if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN + 1) {
+ if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN) {
ret = -EINVAL;
goto out;
}
--
1.8.3.1
--
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/
[PATCH] wlags49_h2: Fix overflow in wireless_set_essid()
This patch prevents the wireless_set_essid() function from overwriting
the last byte of the NetworkName buffer which must be NULL.
---
drivers/staging/wlags49_h2/wl_wext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wlags49_h2/wl_wext.c
b/drivers/staging/wlags49_h2/wl_wext.c
index c731ff2..62ad158 100644
--- a/drivers/staging/wlags49_h2/wl_wext.c
+++ b/drivers/staging/wlags49_h2/wl_wext.c
@@ -1127,7 +1127,7 @@ static int wireless_set_essid(struct net_device *dev,
struct iw_request_info *in
goto out;
}
- if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN + 1) {
+ if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN) {
ret = -EINVAL;
goto out;
}
--
1.8.3.1
--
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/
[PATCH v2] wlags49_h2: Fix overflow in wireless_set_essid()
This patch prevents the wireless_set_essid() function from overwriting
the last byte of the NetworkName buffer which must be NULL.
Signed-off-by: Maurizio Lombardi
---
drivers/staging/wlags49_h2/wl_wext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wlags49_h2/wl_wext.c
b/drivers/staging/wlags49_h2/wl_wext.c
index c731ff2..62ad158 100644
--- a/drivers/staging/wlags49_h2/wl_wext.c
+++ b/drivers/staging/wlags49_h2/wl_wext.c
@@ -1127,7 +1127,7 @@ static int wireless_set_essid(struct net_device *dev,
struct iw_request_info *in
goto out;
}
- if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN + 1) {
+ if (data->flags != 0 && data->length > HCF_MAX_NAME_LEN) {
ret = -EINVAL;
goto out;
}
--
1.8.3.1
--
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/
[PATCH] kobject: fix memory leak in kobject_set_name_vargs
If the call to kvasprintf fails then the old name of the object will be leaked,
this patch fixes the bug by restoring the old name before returning ENOMEM.
Signed-off-by: Maurizio Lombardi
---
lib/kobject.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index 5b4b888..c2cb934 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -247,8 +247,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const
char *fmt,
return 0;
kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
- if (!kobj->name)
+ if (!kobj->name) {
+ kobj->name = old_name;
return -ENOMEM;
+ }
/* ewww... some of these buggers have '/' in the name ... */
while ((s = strchr(kobj->name, '/')))
--
Maurizio Lombardi
--
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/
[PATCH] kobject: fix memory leak in kobject_set_name_vargs
If the call to kvasprintf fails then the old name of the object will be leaked,
this patch fixes the bug by restoring the old name before returning ENOMEM.
Signed-off-by: Maurizio Lombardi
---
lib/kobject.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index 5b4b888..c2cb934 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -247,8 +247,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const
char *fmt,
return 0;
kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
- if (!kobj->name)
+ if (!kobj->name) {
+ kobj->name = old_name;
return -ENOMEM;
+ }
/* ewww... some of these buggers have '/' in the name ... */
while ((s = strchr(kobj->name, '/')))
--
Maurizio Lombardi
--
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/
