| Message ID | 551D60BE.1070101@plexistor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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().
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
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.
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.
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
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.
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
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);