pmem: Add prints at module load and unload
diff mbox

Message ID 551D60BE.1070101@plexistor.com
State New, archived
Headers show

Commit Message

Boaz Harrosh April 2, 2015, 3:31 p.m. UTC
Hi Christoph, Ingo

Please consider this small patch below just a small print at module
load/unload so to know at user systems how things progressed.
As it is now, we know nothing. For any other disk kind we have two
tuns of prints.

---
From: Boaz Harrosh <boaz@plexistor.com>
Date: Thu, 2 Apr 2015 16:43:48 +0300
Subject: [PATCH] pmem: Add prints at module load and unload

When debugging people's systems it is helpful
to see what went on. The load and unload of pmem is
an important event.

The importance is the number of loaded devices and
error status. The exact device's addresses created
we can see from the other prints at e820 so no need
to duplicate this information.

While at it fix up a small issue with rw flags.

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 drivers/block/pmem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Dan Williams April 2, 2015, 3:39 p.m. UTC | #1
On Thu, Apr 2, 2015 at 8:31 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
> Hi Christoph, Ingo
>
> Please consider this small patch below just a small print at module
> load/unload so to know at user systems how things progressed.
> As it is now, we know nothing. For any other disk kind we have two
> tuns of prints.
>
> ---
> From: Boaz Harrosh <boaz@plexistor.com>
> Date: Thu, 2 Apr 2015 16:43:48 +0300
> Subject: [PATCH] pmem: Add prints at module load and unload
>
> When debugging people's systems it is helpful
> to see what went on. The load and unload of pmem is
> an important event.
>
> The importance is the number of loaded devices and
> error status. The exact device's addresses created
> we can see from the other prints at e820 so no need
> to duplicate this information.
>
> While at it fix up a small issue with rw flags.

Separate patch for fixes please.

>
> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
> ---
>  drivers/block/pmem.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
> index 988f384..3f15fbc 100644
> --- a/drivers/block/pmem.c
> +++ b/drivers/block/pmem.c
> @@ -91,8 +91,9 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector,
>  {
>         struct pmem_device *pmem = bdev->bd_disk->private_data;
>
> +       rw &= WRITE;
>         pmem_do_bvec(pmem, page, PAGE_CACHE_SIZE, 0, rw, sector);
> -       page_endio(page, rw & WRITE, 0);
> +       page_endio(page, rw, 0);
>
>         return 0;
>  }
> @@ -248,6 +249,9 @@ static int __init pmem_init(void)
>         error = platform_driver_register(&pmem_driver);
>         if (error)
>                 unregister_blkdev(pmem_major, "pmem");
> +
> +       pr_info("pmem: init %d devices => %d\n",
> +               atomic_read(&pmem_index), error);

If anything I think these should be dev_dbg().
Boaz Harrosh April 2, 2015, 3:47 p.m. UTC | #2
On 04/02/2015 06:39 PM, Dan Williams wrote:
> On Thu, Apr 2, 2015 at 8:31 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>> Hi Christoph, Ingo
>>
>> Please consider this small patch below just a small print at module
>> load/unload so to know at user systems how things progressed.
>> As it is now, we know nothing. For any other disk kind we have two
>> tuns of prints.
>>
>> ---
>> From: Boaz Harrosh <boaz@plexistor.com>
>> Date: Thu, 2 Apr 2015 16:43:48 +0300
>> Subject: [PATCH] pmem: Add prints at module load and unload
>>
>> When debugging people's systems it is helpful
>> to see what went on. The load and unload of pmem is
>> an important event.
>>
>> The importance is the number of loaded devices and
>> error status. The exact device's addresses created
>> we can see from the other prints at e820 so no need
>> to duplicate this information.
>>
>> While at it fix up a small issue with rw flags.
> 
> Separate patch for fixes please.

Sigh, OK I was preparing this and hopping it would just
be SQUASHED into the original patch but Ingo bit me to it
and already submitted the patches.

> 
>>
>> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
>> ---
>>  drivers/block/pmem.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
>> index 988f384..3f15fbc 100644
>> --- a/drivers/block/pmem.c
>> +++ b/drivers/block/pmem.c
>> @@ -91,8 +91,9 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector,
>>  {
>>         struct pmem_device *pmem = bdev->bd_disk->private_data;
>>
>> +       rw &= WRITE;
>>         pmem_do_bvec(pmem, page, PAGE_CACHE_SIZE, 0, rw, sector);
>> -       page_endio(page, rw & WRITE, 0);
>> +       page_endio(page, rw, 0);
>>
>>         return 0;
>>  }
>> @@ -248,6 +249,9 @@ static int __init pmem_init(void)
>>         error = platform_driver_register(&pmem_driver);
>>         if (error)
>>                 unregister_blkdev(pmem_major, "pmem");
>> +
>> +       pr_info("pmem: init %d devices => %d\n",
>> +               atomic_read(&pmem_index), error);
> 
> If anything I think these should be dev_dbg().

We do not have a dev at any of this point, and it does not
belong to any specific device. Also I would like this
_info and not _dbg so to always have it, also for production.
See the chatter for a single SCSI disk the minimum we need
is just the small print that tells all that we need (for now)

Please consider as is?

Thanks
Boaz
Dan Williams April 2, 2015, 4:01 p.m. UTC | #3
On Thu, Apr 2, 2015 at 8:47 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
> On 04/02/2015 06:39 PM, Dan Williams wrote:
>> On Thu, Apr 2, 2015 at 8:31 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>>> Hi Christoph, Ingo
>>>
>>> Please consider this small patch below just a small print at module
>>> load/unload so to know at user systems how things progressed.
>>> As it is now, we know nothing. For any other disk kind we have two
>>> tuns of prints.
>>>
>>> ---
>>> From: Boaz Harrosh <boaz@plexistor.com>
>>> Date: Thu, 2 Apr 2015 16:43:48 +0300
>>> Subject: [PATCH] pmem: Add prints at module load and unload
>>>
>>> When debugging people's systems it is helpful
>>> to see what went on. The load and unload of pmem is
>>> an important event.
>>>
>>> The importance is the number of loaded devices and
>>> error status. The exact device's addresses created
>>> we can see from the other prints at e820 so no need
>>> to duplicate this information.
>>>
>>> While at it fix up a small issue with rw flags.
>>
>> Separate patch for fixes please.
>
> Sigh, OK I was preparing this and hopping it would just
> be SQUASHED into the original patch but Ingo bit me to it
> and already submitted the patches.
>
>>
>>>
>>> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
>>> ---
>>>  drivers/block/pmem.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
>>> index 988f384..3f15fbc 100644
>>> --- a/drivers/block/pmem.c
>>> +++ b/drivers/block/pmem.c
>>> @@ -91,8 +91,9 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector,
>>>  {
>>>         struct pmem_device *pmem = bdev->bd_disk->private_data;
>>>
>>> +       rw &= WRITE;
>>>         pmem_do_bvec(pmem, page, PAGE_CACHE_SIZE, 0, rw, sector);
>>> -       page_endio(page, rw & WRITE, 0);
>>> +       page_endio(page, rw, 0);
>>>
>>>         return 0;
>>>  }
>>> @@ -248,6 +249,9 @@ static int __init pmem_init(void)
>>>         error = platform_driver_register(&pmem_driver);
>>>         if (error)
>>>                 unregister_blkdev(pmem_major, "pmem");
>>> +
>>> +       pr_info("pmem: init %d devices => %d\n",
>>> +               atomic_read(&pmem_index), error);
>>
>> If anything I think these should be dev_dbg().
>
> We do not have a dev at any of this point, and it does not
> belong to any specific device.

Ah, true this is prior to the driver attaching... that said it seems
more relevant to print from probe() (where we do have a device) than
init where the device may remain idle due to some other policy.

> Also I would like this
> _info and not _dbg so to always have it, also for production.
> See the chatter for a single SCSI disk the minimum we need
> is just the small print that tells all that we need (for now)

Not sure we want to follow so closely in the footsteps of SCSI's chattiness.
Christoph Hellwig April 2, 2015, 4:44 p.m. UTC | #4
On Thu, Apr 02, 2015 at 09:01:14AM -0700, Dan Williams wrote:
> >> If anything I think these should be dev_dbg().
> >
> > We do not have a dev at any of this point, and it does not
> > belong to any specific device.
> 
> Ah, true this is prior to the driver attaching... that said it seems
> more relevant to print from probe() (where we do have a device) than
> init where the device may remain idle due to some other policy.
> 
> > Also I would like this
> > _info and not _dbg so to always have it, also for production.
> > See the chatter for a single SCSI disk the minimum we need
> > is just the small print that tells all that we need (for now)
> 
> Not sure we want to follow so closely in the footsteps of SCSI's chattiness.

Defintively not!  A single dev_info at ->probe time sounds ok, something
like:

	dev_info(dev, "registering region [0x%pa:0x%zx]\n",
			&pmem->phys_addr, pmem->size);

but there are plenty other drivers not that chatty, e.g. virtio and we're
doing just fine for them.
Boaz Harrosh April 5, 2015, 8:50 a.m. UTC | #5
On 04/02/2015 07:44 PM, Christoph Hellwig wrote:
> On Thu, Apr 02, 2015 at 09:01:14AM -0700, Dan Williams wrote:
>>>> If anything I think these should be dev_dbg().
>>>
>>> We do not have a dev at any of this point, and it does not
>>> belong to any specific device.
>>
>> Ah, true this is prior to the driver attaching... that said it seems
>> more relevant to print from probe() (where we do have a device) than
>> init where the device may remain idle due to some other policy.
>>
>>> Also I would like this
>>> _info and not _dbg so to always have it, also for production.
>>> See the chatter for a single SCSI disk the minimum we need
>>> is just the small print that tells all that we need (for now)
>>
>> Not sure we want to follow so closely in the footsteps of SCSI's chattiness.
> 
> Defintively not!  A single dev_info at ->probe time sounds ok, something
> like:
> 
> 	dev_info(dev, "registering region [0x%pa:0x%zx]\n",
> 			&pmem->phys_addr, pmem->size);
> 
> but there are plenty other drivers not that chatty, e.g. virtio and we're
> doing just fine for them.
> 

For me, my print is just the exact amount.

So I will see in my dmesg:

[  +0.000000] user: [mem 0x0000000100000000-0x000000015fffffff] persistent (type 12)
[  +0.000000] user: [mem 0x0000000160000000-0x00000001dfffffff] persistent (type 12)

...
[  +0.000537] pmem: init 2 devices => 0

So I have all the information. And I know the driver was actually loaded
successfully on the expected two regions.

Your print above will give me two prints with information I already have.
All I want to know from users logs is that the driver was actually loaded
successful or not. And when it was unloaded. I think this is the bear minimum
that gives me all the information I need. Less then this I would be missing
a very important event.

Thanks
Boaz
Christoph Hellwig April 7, 2015, 3:19 p.m. UTC | #6
On Sun, Apr 05, 2015 at 11:50:06AM +0300, Boaz Harrosh wrote:
> [  +0.000537] pmem: init 2 devices => 0
> 
> So I have all the information. And I know the driver was actually loaded
> successfully on the expected two regions.

The second number will always be 0, so no point in printing it.
Also device can be hotplugged at runtime, e.g. your magic PCIe device,
so iff you really want to print anything ->probe is the place for it.

But I still don't think we need it, once booted you can trivially look
up the information in sysfs.
Boaz Harrosh April 7, 2015, 3:34 p.m. UTC | #7
On 04/07/2015 06:19 PM, Christoph Hellwig wrote:
> On Sun, Apr 05, 2015 at 11:50:06AM +0300, Boaz Harrosh wrote:
>> [  +0.000537] pmem: init 2 devices => 0
>>
>> So I have all the information. And I know the driver was actually loaded
>> successfully on the expected two regions.
> 
> The second number will always be 0, so no point in printing it.

Why it can be any error that was collected in the load process its
the error we are returning to the Kernel from pmem_init()
Any none zero will mean module will not load and modprobe will
return with an exit code.

(Errors from probe() will be returned here)

> Also device can be hotplugged at runtime, e.g. your magic PCIe device,
> so iff you really want to print anything ->probe is the place for it.
> 

Yes but for now it is only very much static. We could add it later
when that happens, no?

> But I still don't think we need it, once booted you can trivially look
> up the information in sysfs.
> 

Its not for the systems I can probe around in sysfs and or just ls /dev/pmem*

It is postmortem when all I have is the logs and say a stack trace.
For us in the lab it is very important, all we need is the single
line on load/unload. But at probe time is fine as well, just more
chatty as you were complaining.

I will post two versions of this right away, I did it and forgot
to post.

Thanks
Boaz

Patch
diff mbox

diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
index 988f384..3f15fbc 100644
--- a/drivers/block/pmem.c
+++ b/drivers/block/pmem.c
@@ -91,8 +91,9 @@  static int pmem_rw_page(struct block_device *bdev, sector_t sector,
 {
 	struct pmem_device *pmem = bdev->bd_disk->private_data;
 
+	rw &= WRITE;
 	pmem_do_bvec(pmem, page, PAGE_CACHE_SIZE, 0, rw, sector);
-	page_endio(page, rw & WRITE, 0);
+	page_endio(page, rw, 0);
 
 	return 0;
 }
@@ -248,6 +249,9 @@  static int __init pmem_init(void)
 	error = platform_driver_register(&pmem_driver);
 	if (error)
 		unregister_blkdev(pmem_major, "pmem");
+
+	pr_info("pmem: init %d devices => %d\n",
+		atomic_read(&pmem_index), error);
 	return error;
 }
 module_init(pmem_init);
@@ -256,6 +260,7 @@  static void pmem_exit(void)
 {
 	platform_driver_unregister(&pmem_driver);
 	unregister_blkdev(pmem_major, "pmem");
+	pr_info("pmem: exit\n");
 }
 module_exit(pmem_exit);