Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1202143 > unrolled thread

[PATCH 0/3] zram: Replace pr_* with dev_*

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] zram: Replace pr_* with dev_* Salah Triki <salah.triki@acm.org> - 2015-08-07 01:00 +0200
    [PATCH 3/3] zram: Replace pr_* with dev_* Salah Triki <salah.triki@acm.org> - 2015-08-07 01:10 +0200
    [PATCH 2/3] zram: Replace pr_info with dev_info in comp_algorithm_store Salah Triki <salah.triki@acm.org> - 2015-08-07 01:10 +0200
    [PATCH 1/3] zram: Replace pr_info with dev_info in max_comp_streams_store Salah Triki <salah.triki@acm.org> - 2015-08-07 01:10 +0200
    Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-07 02:10 +0200
      Re: [PATCH 0/3] zram: Replace pr_* with dev_* Joe Perches <joe@perches.com> - 2015-08-07 03:20 +0200
        Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-07 03:50 +0200
          Re: [PATCH 0/3] zram: Replace pr_* with dev_* Joe Perches <joe@perches.com> - 2015-08-07 03:50 +0200
            Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-07 04:10 +0200
              Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-07 04:20 +0200
      Re: [PATCH 0/3] zram: Replace pr_* with dev_* Minchan Kim <minchan@kernel.org> - 2015-08-07 08:10 +0200
        Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-07 08:40 +0200
          Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-07 09:00 +0200
            Re: [PATCH 0/3] zram: Replace pr_* with dev_* Joe Perches <joe@perches.com> - 2015-08-07 09:20 +0200
              Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-07 09:30 +0200
          Re: [PATCH 0/3] zram: Replace pr_* with dev_* Minchan Kim <minchan@kernel.org> - 2015-08-07 17:00 +0200
            Re: [PATCH 0/3] zram: Replace pr_* with dev_* Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-10 03:30 +0200

#1202143 — [PATCH 0/3] zram: Replace pr_* with dev_*

This patchset replaces pr_* with dev_*. dev_* attach kernel messages to the right
device. In addition, patchs 1 and 2 add to messages the values of variables that trigger 
errors.

Salah Triki (3):
  zram: Replace pr_info with dev_info in max_comp_streams_store
  zram: Replace pr_info with dev_info in comp_algorithm_store
  zram: Replace pr_* with dev_*

 drivers/block/zram/zram_drv.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1202148 — [PATCH 3/3] zram: Replace pr_* with dev_*

dev_* attach the messages to the devices.
---
 drivers/block/zram/zram_drv.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 1c2f8b9..73f3453 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -589,7 +589,8 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret)) {
-		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
+		dev_err(disk_to_dev(zram->disk), "Decompression failed! err=%d, page=%u\n",
+			ret, index);
 		return ret;
 	}
 
@@ -623,7 +624,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 		uncmem = user_mem;
 
 	if (!uncmem) {
-		pr_info("Unable to allocate temp memory\n");
+		dev_info(disk_to_dev(zram->disk), "Unable to allocate temp memory\n");
 		ret = -ENOMEM;
 		goto out_cleanup;
 	}
@@ -708,7 +709,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	}
 
 	if (unlikely(ret)) {
-		pr_err("Compression failed! err=%d\n", ret);
+		dev_err(disk_to_dev(zram->disk), "Compression failed! err=%d\n",
+			ret);
 		goto out;
 	}
 	src = zstrm->buffer;
@@ -720,7 +722,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 
 	handle = zs_malloc(meta->mem_pool, clen);
 	if (!handle) {
-		pr_info("Error allocating memory for compressed page: %u, size=%zu\n",
+		dev_info(disk_to_dev(zram->disk), "Error allocating memory for compressed page: %u, size=%zu\n",
 			index, clen);
 		ret = -ENOMEM;
 		goto out;
@@ -1039,15 +1041,15 @@ static ssize_t disksize_store(struct device *dev,
 
 	comp = zcomp_create(zram->compressor, zram->max_comp_streams);
 	if (IS_ERR(comp)) {
-		pr_info("Cannot initialise %s compressing backend\n",
-				zram->compressor);
+		dev_info(dev, "Cannot initialise %s compressing backend\n",
+			zram->compressor);
 		err = PTR_ERR(comp);
 		goto out_free_meta;
 	}
 
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
-		pr_info("Cannot change disksize for initialized device\n");
+		dev_info(dev, "Cannot change disksize for initialized device\n");
 		err = -EBUSY;
 		goto out_destroy_comp;
 	}
@@ -1206,7 +1208,7 @@ static int zram_add(void)
 
 	queue = blk_alloc_queue(GFP_KERNEL);
 	if (!queue) {
-		pr_err("Error allocating disk queue for device %d\n",
+		dev_err(disk_to_dev(zram->disk), "Error allocating disk queue for device %d\n",
 			device_id);
 		ret = -ENOMEM;
 		goto out_free_idr;
@@ -1217,7 +1219,7 @@ static int zram_add(void)
 	/* gendisk structure */
 	zram->disk = alloc_disk(1);
 	if (!zram->disk) {
-		pr_warn("Error allocating disk structure for device %d\n",
+		dev_warn(disk_to_dev(zram->disk), "Error allocating disk structure for device %d\n",
 			device_id);
 		ret = -ENOMEM;
 		goto out_free_queue;
@@ -1266,14 +1268,15 @@ static int zram_add(void)
 	ret = sysfs_create_group(&disk_to_dev(zram->disk)->kobj,
 				&zram_disk_attr_group);
 	if (ret < 0) {
-		pr_warn("Error creating sysfs group");
+		dev_warn(disk_to_dev(zram->disk), "Error creating sysfs group");
 		goto out_free_disk;
 	}
 	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
 	zram->meta = NULL;
 	zram->max_comp_streams = 1;
 
-	pr_info("Added device: %s\n", zram->disk->disk_name);
+	dev_info(disk_to_dev(zram->disk), "Added device: %s\n",
+		zram->disk->disk_name);
 	return device_id;
 
 out_free_disk:
@@ -1321,7 +1324,8 @@ static int zram_remove(struct zram *zram)
 	zram_reset_device(zram);
 	bdput(bdev);
 
-	pr_info("Removed device: %s\n", zram->disk->disk_name);
+	dev_info(disk_to_dev(zram->disk), "Removed device: %s\n",
+		zram->disk->disk_name);
 
 	idr_remove(&zram_index_idr, zram->disk->first_minor);
 	blk_cleanup_queue(zram->disk->queue);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202150 — [PATCH 2/3] zram: Replace pr_info with dev_info in comp_algorithm_store

dev_info attachs the message to the device. And, the algorithm's name, that
triggers the error, is added to the message.
---
 drivers/block/zram/zram_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index bb284db..1c2f8b9 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -369,7 +369,8 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
 		up_write(&zram->init_lock);
-		pr_info("Can't change algorithm for initialized device\n");
+		dev_info(dev, "Can't change algorithm to %s for initialized device\n",
+			 buf);
 		return -EBUSY;
 	}
 	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202152 — [PATCH 1/3] zram: Replace pr_info with dev_info in max_comp_streams_store

dev_info attachs the message to the device. And, the max compression
streams value, that triggers error, is added to the message.
---
 drivers/block/zram/zram_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index fb655e8..bb284db 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -333,7 +333,8 @@ static ssize_t max_comp_streams_store(struct device *dev,
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
 		if (!zcomp_set_max_streams(zram->comp, num)) {
-			pr_info("Cannot change max compression streams\n");
+			dev_info(dev, "Cannot change max compression streams to %d\n",
+				num);
 			ret = -EINVAL;
 			goto out;
 		}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202168

On (08/07/15 00:03), Salah Triki wrote:
> This patchset replaces pr_* with dev_*. dev_* attach kernel messages to the right
> device. In addition, patchs 1 and 2 add to messages the values of variables that trigger 
> errors.
> 

Hi,

I'd prefer to leave the messages the way they are. Changing anything
visible to user space (api, eror codes, error messages, etc.) is a
very risky business. You change the format of error messages and it
smells like a big NO-NO.

'zram: Cannot initialise lzo compressing backend'
--> 'block zram0: Cannot initialise lzo compressing backend'


And there are even more dramatic changes:
"Cannot change max compression streams\n"
--> "Cannot change max compression streams to %d\n"

"Can't change algorithm for initialized device\n"
--> "Can't change algorithm to %s for initialized device\n"


People already can have scripts doing `grep "zram:"` on dmesg or
whatever. We cannot change this anymore.

This potentially breaks things in user space. So, I NACK the change
set. Thanks.

Minchan, any opinion?

	-ss

> Salah Triki (3):
>   zram: Replace pr_info with dev_info in max_comp_streams_store
>   zram: Replace pr_info with dev_info in comp_algorithm_store
>   zram: Replace pr_* with dev_*
> 
>  drivers/block/zram/zram_drv.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202183

On Fri, 2015-08-07 at 09:05 +0900, Sergey Senozhatsky wrote:
> On (08/07/15 00:03), Salah Triki wrote:
> > This patchset replaces pr_* with dev_*. dev_* attach kernel messages to the right
> > device. In addition, patchs 1 and 2 add to messages the values of variables that trigger 
> > errors.
[]
> I'd prefer to leave the messages the way they are. Changing anything
> visible to user space (api, eror codes, error messages, etc.) is a
> very risky business. You change the format of error messages and it
> smells like a big NO-NO.
> 
> 'zram: Cannot initialise lzo compressing backend'
> --> 'block zram0: Cannot initialise lzo compressing backend'
> 
> 
> And there are even more dramatic changes:
> "Cannot change max compression streams\n"
> --> "Cannot change max compression streams to %d\n"
> 
> "Can't change algorithm for initialized device\n"
> --> "Can't change algorithm to %s for initialized device\n"
> 
> 
> People already can have scripts doing `grep "zram:"` on dmesg or
> whatever. We cannot change this anymore.

That's not true at all.

Using grep on dmesg is specifically _not_ guaranteed
to remain stable between kernel versions.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202195

Hello Joe,

On (08/06/15 18:17), Joe Perches wrote:
[..]
> > "Can't change algorithm for initialized device\n"
> > --> "Can't change algorithm to %s for initialized device\n"
> > 
> > 
> > People already can have scripts doing `grep "zram:"` on dmesg or
> > whatever. We cannot change this anymore.
> 
> That's not true at all.
> 
> Using grep on dmesg is specifically _not_ guaranteed
> to remain stable between kernel versions.

It depends, I guess.  People do use grep after all and people don't
like when things are getting changed underneath; and we don't want
to do this. I think Minchan is with me here. We even didn't add some
additional pr_info/pr_err noise recently because we don't want
people to depend on that part.

http://lkml.iu.edu/hypermail/linux/kernel/1505.3/01759.html

Minchan Kim <minchan@kernel.org>:

|I meant if we remove the pr_err in future by some reason,
|someone might shout
|
|"No, it's ABI so if you guys removes it, it will break user interface's
|semantic". Maybe he seems to depends on parse on dmesg.
|That is not what I want.

And I saw some time ago people doing that type of thing. So I'd like
to avoid unnecessary pain for zram users even if the messages are not
guaranteed to remain stable between kernel releases. Just my opinion.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202196

On Fri, 2015-08-07 at 10:42 +0900, Sergey Senozhatsky wrote:
> Hello Joe,
> 
> On (08/06/15 18:17), Joe Perches wrote:
> [..]
> > > "Can't change algorithm for initialized device\n"
> > > --> "Can't change algorithm to %s for initialized device\n"
> > > 
> > > 
> > > People already can have scripts doing `grep "zram:"` on dmesg or
> > > whatever. We cannot change this anymore.
> > 
> > That's not true at all.
> > 
> > Using grep on dmesg is specifically _not_ guaranteed
> > to remain stable between kernel versions.
> 
> It depends, I guess.  People do use grep after all and people don't
> like when things are getting changed underneath; and we don't want
> to do this. I think Minchan is with me here. We even didn't add some
> additional pr_info/pr_err noise recently because we don't want
> people to depend on that part.
> 
> http://lkml.iu.edu/hypermail/linux/kernel/1505.3/01759.html
> 
> Minchan Kim <minchan@kernel.org>:
> 
> |I meant if we remove the pr_err in future by some reason,
> |someone might shout
> |
> |"No, it's ABI so if you guys removes it, it will break user interface's
> |semantic". Maybe he seems to depends on parse on dmesg.
> |That is not what I want.
> 
> And I saw some time ago people doing that type of thing. So I'd like
> to avoid unnecessary pain for zram users even if the messages are not
> guaranteed to remain stable between kernel releases. Just my opinion.

I'm fine with you having an opinion but I'm not fine
with you stating:

"We cannot change this anymore.

This potentially breaks things in user space.
So, I NACK the change set."
.
because dmesg is not an ABI.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202228

On (08/06/15 18:48), Joe Perches wrote:
[..]
> > And I saw some time ago people doing that type of thing. So I'd like
> > to avoid unnecessary pain for zram users even if the messages are not
> > guaranteed to remain stable between kernel releases. Just my opinion.

[..]
> because dmesg is not an ABI.

I absolutely agree, this is debatable and controversial. I saw people
grepping dmesg and they treated error messages just like errno codes or
like additional info to attach to errno. Whether we like or not, but, it
seems, for that cases error message is more or less part of ABI. It's
fine when we change _info message, but touching _warn or _err message
is a bit different thing, *I think*. Let's hear from Minchan.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202241

On (08/07/15 11:03), Sergey Senozhatsky wrote:
> [..]
> > because dmesg is not an ABI.
> 
> I absolutely agree, this is debatable and controversial. I saw people
> grepping dmesg and they treated error messages just like errno codes or
> like additional info to attach to errno. Whether we like or not, but, it
> seems, for that cases error message is more or less part of ABI. It's
> fine when we change _info message, but touching _warn or _err message
> is a bit different thing, *I think*. Let's hear from Minchan.
> 

A side note (not to forget).

If Minchan agress that messages are free come and go, then I'd probably
like to remove this one

         meta->table = vzalloc(num_pages * sizeof(*meta->table));
         if (!meta->table) {
                 pr_err("Error allocating zram address table\n");
			^^^^^^
                 goto out_error;
         }

because __vmalloc_node_range() will warn_alloc_failed() anyway.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202305

Hello,

On Fri, Aug 07, 2015 at 09:05:20AM +0900, Sergey Senozhatsky wrote:
> On (08/07/15 00:03), Salah Triki wrote:
> > This patchset replaces pr_* with dev_*. dev_* attach kernel messages to the right
> > device. In addition, patchs 1 and 2 add to messages the values of variables that trigger 
> > errors.
> > 
> 
> Hi,
> 
> I'd prefer to leave the messages the way they are. Changing anything
> visible to user space (api, eror codes, error messages, etc.) is a
> very risky business. You change the format of error messages and it
> smells like a big NO-NO.
> 
> 'zram: Cannot initialise lzo compressing backend'
> --> 'block zram0: Cannot initialise lzo compressing backend'
> 
> 
> And there are even more dramatic changes:
> "Cannot change max compression streams\n"
> --> "Cannot change max compression streams to %d\n"
> 
> "Can't change algorithm for initialized device\n"
> --> "Can't change algorithm to %s for initialized device\n"
> 
> 
> People already can have scripts doing `grep "zram:"` on dmesg or
> whatever. We cannot change this anymore.
> 
> This potentially breaks things in user space. So, I NACK the change
> set. Thanks.
> 
> Minchan, any opinion?

Note: I didn't read this patchset in detail so I might be wrong.

When I read description, I couldn't see what's the benefit.
Please write it out.

As well, please write how you change current message. IOW,
before and after.

If the benefit is not enough for me, I don't want to change.

Thanks.

> 
> 	-ss
> 
> > Salah Triki (3):
> >   zram: Replace pr_info with dev_info in max_comp_streams_store
> >   zram: Replace pr_info with dev_info in comp_algorithm_store
> >   zram: Replace pr_* with dev_*
> > 
> >  drivers/block/zram/zram_drv.c | 34 ++++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> > 
> > -- 
> > 1.9.1
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202308

Hello Minchan,

On (08/07/15 15:05), Minchan Kim wrote:
[..]
> > I'd prefer to leave the messages the way they are. Changing anything
> > visible to user space (api, eror codes, error messages, etc.) is a
> > very risky business. You change the format of error messages and it
> > smells like a big NO-NO.
> > 
> > 'zram: Cannot initialise lzo compressing backend'
> > --> 'block zram0: Cannot initialise lzo compressing backend'
> > 
> > 
> > And there are even more dramatic changes:
> > "Cannot change max compression streams\n"
> > --> "Cannot change max compression streams to %d\n"
> > 
> > "Can't change algorithm for initialized device\n"
> > --> "Can't change algorithm to %s for initialized device\n"
> > 
> > 
> > People already can have scripts doing `grep "zram:"` on dmesg or
> > whatever. We cannot change this anymore.
> > 
> > This potentially breaks things in user space. So, I NACK the change
> > set. Thanks.
> > 
> > Minchan, any opinion?
> 
> Note: I didn't read this patchset in detail so I might be wrong.
> 
> When I read description, I couldn't see what's the benefit.
> Please write it out.

we now have errors like
 'zram: Cannot initialise lzo compressing backend'

and they will transform into

 'block zram0: Cannot initialise lzo compressing backend'

note the prefix 'zram:' became 'block zram0:'


and there are two patches (well, at least I quickly spotted only
those) that change messages' text

From:  "Cannot change max compression streams\n"
To: "Cannot change max compression streams to %d\n"

where %d is a supplied max_comp_stream value

From: "Can't change algorithm for initialized device\n"
To: "Can't change algorithm to %s for initialized device\n"

where %s is a supplied compression algorithm name.


as far as I can tell.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202324

On (08/07/15 15:37), Sergey Senozhatsky wrote:
[..]
> we now have errors like
>  'zram: Cannot initialise lzo compressing backend'
> 
> and they will transform into
> 
>  'block zram0: Cannot initialise lzo compressing backend'
> 
> note the prefix 'zram:' became 'block zram0:'

but it doesn't come for free. where we had clean and nice

pr_err("Decompression failed!...
pr_info("Unable to allocate temp memory\n"...
etc...

now we have monsters

dev_err(disk_to_dev(zram->disk), "Decompression failed!...
dev_info(disk_to_dev(zram->disk), "Unable to allocate temp memory\n"...
etc.


other changes are very questionable... for example

   pr_info("Added device: %s\n", zram->disk->disk_name);
becomes
   dev_info(disk_to_dev(zram->disk), "Added device: %s\n", zram->disk->disk_name);


why? there is no reason to do this!



and messages are converted in a bit random manner, shall I say. so now
we have a mix of dev_* and pr_* errors. to convert them all to dev_* (which
is not possible in all the cases) we would need to change some function
prototypes and start passing zram pointer, or disk...

example:

static struct zram_meta *zram_meta_alloc(int device_id, u64 disksize)
{
        size_t num_pages;
        char pool_name[8];
        struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);

        if (!meta)
                return NULL;

        num_pages = disksize >> PAGE_SHIFT;
        meta->table = vzalloc(num_pages * sizeof(*meta->table));
        if (!meta->table) {
                pr_err("Error allocating zram address table\n");
                goto out_error;
        }

        snprintf(pool_name, sizeof(pool_name), "zram%d", device_id);
        meta->mem_pool = zs_create_pool(pool_name, GFP_NOIO | __GFP_HIGHMEM);
        if (!meta->mem_pool) {
                pr_err("Error creating memory pool\n");
                goto out_error;
        }
...



so I see a little value. really. too much things to change.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202332

On Fri, 2015-08-07 at 15:56 +0900, Sergey Senozhatsky wrote:
> On (08/07/15 15:37), Sergey Senozhatsky wrote:
> [..]
> where we had clean and nice
> 
> pr_err("Decompression failed!...
> pr_info("Unable to allocate temp memory\n"...
> etc...
> 
> now we have monsters
> 
> dev_err(disk_to_dev(zram->disk), "Decompression failed!...
> dev_info(disk_to_dev(zram->disk), "Unable to allocate temp memory\n"...
> etc.
[]
> other changes are very questionable... for example
>    pr_info("Added device: %s\n", zram->disk->disk_name);
> becomes
>    dev_info(disk_to_dev(zram->disk), "Added device: %s\n", zram->disk->disk_name);
> 
> why? there is no reason to do this!

This seems a reasonable complaint.

One option is to add some macros like

#define zram_err(zram, fmt, ...)				\
	dev_err(disk_to_dev((zram)->disk), fmt, ##__VA_ARGS__)

But the overall utility of the proposed changes is
moderately low to non-existent.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202371

On (08/07/15 00:12), Joe Perches wrote:
> On Fri, 2015-08-07 at 15:56 +0900, Sergey Senozhatsky wrote:
> > On (08/07/15 15:37), Sergey Senozhatsky wrote:
> > [..]
> > where we had clean and nice
> > 
> > pr_err("Decompression failed!...
> > pr_info("Unable to allocate temp memory\n"...
> > etc...
> > 
> > now we have monsters
> > 
> > dev_err(disk_to_dev(zram->disk), "Decompression failed!...
> > dev_info(disk_to_dev(zram->disk), "Unable to allocate temp memory\n"...
> > etc.
> []
> > other changes are very questionable... for example
> >    pr_info("Added device: %s\n", zram->disk->disk_name);
> > becomes
> >    dev_info(disk_to_dev(zram->disk), "Added device: %s\n", zram->disk->disk_name);
> > 
> > why? there is no reason to do this!
> 
> This seems a reasonable complaint.
> 
> One option is to add some macros like
> 
> #define zram_err(zram, fmt, ...)				\
> 	dev_err(disk_to_dev((zram)->disk), fmt, ##__VA_ARGS__)
> 
> But the overall utility of the proposed changes is
> moderately low to non-existent.

yes, sure. but we still need to change several internal functions
to start accepting struct zram pointer just to be able to show
extra prefix in error messages (if we want the messages to be more
or less consistent).

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202795

On Fri, Aug 07, 2015 at 03:37:56PM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (08/07/15 15:05), Minchan Kim wrote:
> [..]
> > > I'd prefer to leave the messages the way they are. Changing anything
> > > visible to user space (api, eror codes, error messages, etc.) is a
> > > very risky business. You change the format of error messages and it
> > > smells like a big NO-NO.
> > > 
> > > 'zram: Cannot initialise lzo compressing backend'
> > > --> 'block zram0: Cannot initialise lzo compressing backend'
> > > 
> > > 
> > > And there are even more dramatic changes:
> > > "Cannot change max compression streams\n"
> > > --> "Cannot change max compression streams to %d\n"
> > > 
> > > "Can't change algorithm for initialized device\n"
> > > --> "Can't change algorithm to %s for initialized device\n"
> > > 
> > > 
> > > People already can have scripts doing `grep "zram:"` on dmesg or
> > > whatever. We cannot change this anymore.
> > > 
> > > This potentially breaks things in user space. So, I NACK the change
> > > set. Thanks.
> > > 
> > > Minchan, any opinion?
> > 
> > Note: I didn't read this patchset in detail so I might be wrong.
> > 
> > When I read description, I couldn't see what's the benefit.
> > Please write it out.
> 
> we now have errors like
>  'zram: Cannot initialise lzo compressing backend'
> 
> and they will transform into
> 
>  'block zram0: Cannot initialise lzo compressing backend'
> 
> note the prefix 'zram:' became 'block zram0:'

It would be better because it's more clear if we can make several
blocks.

> 
> 
> and there are two patches (well, at least I quickly spotted only
> those) that change messages' text
> 
> From:  "Cannot change max compression streams\n"
> To: "Cannot change max compression streams to %d\n"

In this context, we don't need to say about max_comp_streams value.
It's just fail regardless of the number of max stream you passed.
Instead, it would be better to have prefix like above(ie, block zram0)
for clarification/consistency.

> 
> where %d is a supplied max_comp_stream value
> 
> From: "Can't change algorithm for initialized device\n"
> To: "Can't change algorithm to %s for initialized device\n"

It's okay but hope to have prefix "block zram0"

> 
> where %s is a supplied compression algorithm name.
> 
> 
> as far as I can tell.

Thanks for the explain. Sergey!

I feel we should investigate all of error/log info places to make
them clear/consistent if we decide to go with this.

Thanks.
-- 
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203730

On (08/07/15 23:58), Minchan Kim wrote:
[..]
> > note the prefix 'zram:' became 'block zram0:'
> 
> It would be better because it's more clear if we can make several
> blocks.

in that case I'd rather prefer to add zram%d to some of the existing
messages, than mix pr_* with dev_*. besides, 'block zram0:' is a bit
too long, a short 'zram0:' looks better.

well... we already return -errno from every path that does interact
with user space, which is (I think) good enough; adding device_ids
to those 'error return paths' may be OK.

for example. suppose zram is a swap device and we have !handle case

        handle = zs_malloc(meta->mem_pool, clen);
        if (!handle) {
                pr_info("Error allocating memory for compressed page: %u, size=%zu\n",
                        index, clen);
                ret = -ENOMEM;
                goto out;
        }

if zs_malloc() failed, then the system is in such big troubles, that
device_id in the message is the last thing anyone will look at.


so I'm skeptical. the benefit is sort of minimal.


[..]
> > 
> > where %d is a supplied max_comp_stream value
> > 
> > From: "Can't change algorithm for initialized device\n"
> > To: "Can't change algorithm to %s for initialized device\n"
> 
> It's okay but hope to have prefix "block zram0"

it's not ok. I haven't tested it, but I think that the error message
is broken. the supplied string most likely will contain trailing
\n (I think people usually use echo, not echo -n), so it will be:
	Can't change algorithm to XXX
	 for initialized device

... haven't tested.


BUT why would compression algorithm name even matter here?? the error
is 'you attempted to configure _an already configured device_', not
'you attempted to configure an already configured device _with XXX_'.
that XXX part is not relevant here, 'already configured device' is.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web