[PATCH v6] st implement tape statistics
The following patch exposes statistics for the st driver via sysfs.
There is a need for companies with large numbers of tape drives
numbering in the tens of thousands to track the performance of
those tape drives (e.g. when a backup exceeds its window). The
statistics provided should allow the calculation of throughput,
average block sizes for read and write, and time spent waiting
for I/O to complete or doing tape movement.
Signed-off-by: Shane Seymour
Tested-by: Shane Seymour
---
- Removed comment
- Found an issue where read and write sizes were over reported
(fixed) In all the test cases I have the stats now report what I
expect to be the correct value. Some of the values to be used
with statistics are now stored in temporary variables and used
to calculate the stats when the I/O ends. Separated out the
timestamp into 3 since I found it was possible for other tape
I/O to happen during writes updating the stamp value causing
the time tracked to be wrong.
- Moved the end statistics into a separate function because it
had made the function that it was in too large.
- Added a new statistic - A count of the number of times we had
a residual greater than 0.
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-02-11 22:37:01.382243090 -0600
@@ -471,6 +471,47 @@ static void st_release_request(struct st
kfree(streq);
}
+static void st_do_stats(struct scsi_tape *STp, struct request *req)
+{
+ u64 ticks;
+
+ ticks = get_jiffies_64();
+ STp->stats->in_flight--;
+ if (req->cmd[0] == WRITE_6) {
+ ticks -= STp->stats->write_stamp;
+ STp->stats->write_ticks += ticks;
+ STp->stats->io_ticks += ticks;
+ STp->stats->write_cnt++;
+ if (req->errors) {
+ STp->stats->write_byte_cnt +=
+ STp->stats->last_write_size -
+ STp->buffer->cmdstat.residual;
+ if (STp->buffer->cmdstat.residual > 0)
+ STp->stats->resid_cnt++;
+ } else
+ STp->stats->write_byte_cnt +=
+ STp->stats->last_write_size;
+ } else if (req->cmd[0] == READ_6) {
+ ticks -= STp->stats->read_stamp;
+ STp->stats->read_ticks += ticks;
+ STp->stats->io_ticks += ticks;
+ STp->stats->read_cnt++;
+ if (req->errors)
+ STp->stats->read_byte_cnt +=
+ STp->stats->last_read_size -
+ STp->buffer->cmdstat.residual;
+ if (STp->buffer->cmdstat.residual > 0)
+ STp->stats->resid_cnt++;
+ else
+ STp->stats->read_byte_cnt +=
+ STp->stats->last_read_size;
+ } else {
+ ticks -= STp->stats->other_stamp;
+ STp->stats->io_ticks += ticks;
+ STp->stats->other_cnt++;
+ }
+}
+
static void st_scsi_execute_end(struct request *req, int uptodate)
{
struct st_request *SRpnt = req->end_io_data;
@@ -480,6 +521,8 @@ static void st_scsi_execute_end(struct r
STp->buffer->cmdstat.midlevel_result = SRpnt->result = req->errors;
STp->buffer->cmdstat.residual = req->resid_len;
+ st_do_stats(STp, req);
+
tmp = SRpnt->bio;
if (SRpnt->waiting)
complete(SRpnt->waiting);
@@ -496,6 +539,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = &SRpnt->stp->buffer->map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+ struct scsi_tape *STp = SRpnt->stp;
req = blk_get_request(SRpnt->stp->device->request_queue, write,
GFP_KERNEL);
@@ -516,6 +560,17 @@ static int st_scsi_execute(struct st_req
}
}
+ if (cmd[0] == WRITE_6) {
+ STp->stats->last_write_size = bufflen;
+ STp->stats->write_stamp = get_jiffies_64();
+ } else if (cmd[0] == READ_6) {
+ STp->stats->last_read_size = bufflen;
+ STp->stats->read_stamp = get_jiffies_64();
+ } else {
+ STp->stats->other_stamp = get_jiffies_64();
+ }
+ STp->stats->in_flight++;
+
SRpnt->bio = req->bio;
req->cmd_len = COMMAND_SIZE(cmd[0]);
memset(req->cmd, 0, BLK_MAX_CDB);
@@ -4222,6 +4277,12 @@ static int st_probe(struct device *dev)
}
tpnt->index = error;
sprintf(disk->disk_name, "st%d", tpnt->index);
+ tpnt->stats = kzalloc(sizeof(struct scsi_tape_stats), GFP_KERNEL);
+ if (tpnt->stats == NULL) {
+ sdev_printk(KERN_ERR, SDp,
+ "st: Can't allocate statistics.\n");
+ goto out_idr_remove;
+ }
dev_set_drvdata(
Re: scsi: Implement per-cpu logging buffer
On Wed, Feb 11, 2015 at 8:16 PM, Linux Kernel Mailing List
wrote:
> Gitweb:
> http://git.kernel.org/linus/;a=commit;h=ded85c193a391a84076d5c6a7a5668fe164a490e
> Commit: ded85c193a391a84076d5c6a7a5668fe164a490e
> Parent: b0a93d96b2814c725161f91a4e35d0c29ec0f95b
> Refname:refs/heads/master
> Author: Hannes Reinecke
> AuthorDate: Thu Jan 8 07:43:42 2015 +0100
> Committer: Christoph Hellwig
> CommitDate: Fri Jan 9 15:44:28 2015 +0100
>
> scsi: Implement per-cpu logging buffer
>
> Implement a per-cpu buffer for formatting messages to avoid line breaks
> up under high load. This patch implements scmd_printk() and
> sdev_prefix_printk() using the per-cpu buffer and makes sdev_printk() a
> wrapper for sdev_prefix_printk().
>
> Tested-by: Robert Elliott
> Reviewed-by: Robert Elliott
> Signed-off-by: Hannes Reinecke
> Signed-off-by: Christoph Hellwig
> --- /dev/null
> +++ b/drivers/scsi/scsi_logging.c
> +#define SCSI_LOG_SPOOLSIZE 4096
> +#define SCSI_LOG_BUFSIZE 128
> +
> +#if (SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE) > BITS_PER_LONG
> +#warning SCSI logging bitmask too large
> +#endif
> +
> +struct scsi_log_buf {
> + char buffer[SCSI_LOG_SPOOLSIZE];
> + unsigned long map;
> +};
> +
> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
Do we really need a static 4 KiB per-CPU buffer?
bloat-o-meter:
add/remove: 183/94 grow/shrink: 314/211 up/down: 33467/-21291 (12176)
function old new delta
scsi_format_log-4100 +4100
handle_mm_fault 17942750+956
scsi_log_print_sense_hdr - 774+774
proc_keys_show - 770+770
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
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
Re: [Lsf-pc] [LSF/MM TOPIC] Unifying the LIO and SCST target drivers
On Feb 9, 2:16pm, Bart Van Assche wrote:
} Subject: Re: [Lsf-pc] [LSF/MM TOPIC] Unifying the LIO and SCST target driv
> On 02/03/15 11:06, Dr. Greg Wettstein wrote:
> > It takes a six line patch to the in-kernel Qlogic target driver for
> > SCST to leverage and contribute positively to the state of the
> > in-kernel driver. A six line patch, which if we were engaging in
> > grounded engineering discussions, implements an interface which we
> > haven't found anyone who suggests is unfounded.
> Hi Greg,
Hi Bart, I hope your week is going well.
> Thanks for your feedback. Although I certainly value your work,
> please note that what I proposed goes further than what you have
> done. If I understood your e-mails correctly what you have done is
> to unify the API between the QLogic initiator and target drivers for
> SCST and LIO. My proposal comprises not only a unification of the
> initiator drivers but also of the target drivers.
Just to clarify issues for discussion our work on the SQATGT driver
centered exclusively on the target component of the Qlogic driver.
Our work focused on providing an SCST equivalant to the tcm_qla2xxx.c
driver which the in-kernel implementation of the Qlogic target driver
uses to interface with the TCM target core. The {scst,tcm}_qla2xxx
drivers register a function dispatch structure with the Qlogic
hardware target driver, qla_target.c. These drivers take the actions
requested by the hardware driver and translate them into the necessary
target core processing functions.
As I have indicated the scst_qla2xxx driver could be distributed
separately from the kernel since it only relies on symbols which are
exported from the Qlogic driver. I'm not familiar with IB and such
but it seems to be a reasonable architecture for both camps of
interest to collaborate on.
> Last week I finally found the time to start working on an sample
> implementation of such a unified driver. I hope that I will be able to
> make that code available for review somewhere next week.
Let us know when that becomes available since we would certainly like
to be assisting and supporting whatever the community agrees to as a
solution.
> Bart.
Have a good week.
}-- End of excerpt from Bart Van Assche
As always,
Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC.
4206 N. 19th Ave. Specializing in information infra-structure
Fargo, ND 58102development.
PH: 701-281-1686
FAX: 701-281-3949 EMAIL: g...@enjellic.com
--
"... Doc (interviews) is closest in look&feel to a Windows word-processor.
It's even slow. Very slow. Hard to set up fonts and printing in the
version I have"
-- David Johnson
--
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
Re: scsi: Implement per-cpu logging buffer
On 02/12/2015 01:36 PM, Geert Uytterhoeven wrote:
> On Wed, Feb 11, 2015 at 8:16 PM, Linux Kernel Mailing List
> wrote:
>> Gitweb:
>> http://git.kernel.org/linus/;a=commit;h=ded85c193a391a84076d5c6a7a5668fe164a490e
>> Commit: ded85c193a391a84076d5c6a7a5668fe164a490e
>> Parent: b0a93d96b2814c725161f91a4e35d0c29ec0f95b
>> Refname:refs/heads/master
>> Author: Hannes Reinecke
>> AuthorDate: Thu Jan 8 07:43:42 2015 +0100
>> Committer: Christoph Hellwig
>> CommitDate: Fri Jan 9 15:44:28 2015 +0100
>>
>> scsi: Implement per-cpu logging buffer
>>
>> Implement a per-cpu buffer for formatting messages to avoid line breaks
>> up under high load. This patch implements scmd_printk() and
>> sdev_prefix_printk() using the per-cpu buffer and makes sdev_printk() a
>> wrapper for sdev_prefix_printk().
>>
>> Tested-by: Robert Elliott
>> Reviewed-by: Robert Elliott
>> Signed-off-by: Hannes Reinecke
>> Signed-off-by: Christoph Hellwig
>
>> --- /dev/null
>> +++ b/drivers/scsi/scsi_logging.c
>
>> +#define SCSI_LOG_SPOOLSIZE 4096
>> +#define SCSI_LOG_BUFSIZE 128
>> +
>> +#if (SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE) > BITS_PER_LONG
>> +#warning SCSI logging bitmask too large
>> +#endif
>> +
>> +struct scsi_log_buf {
>> + char buffer[SCSI_LOG_SPOOLSIZE];
>> + unsigned long map;
>> +};
>> +
>> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
>
> Do we really need a static 4 KiB per-CPU buffer?
>
> bloat-o-meter:
>
> add/remove: 183/94 grow/shrink: 314/211 up/down: 33467/-21291 (12176)
> function old new delta
> scsi_format_log-4100 +4100
> handle_mm_fault 17942750+956
> scsi_log_print_sense_hdr - 774+774
> proc_keys_show - 770+770
>
Define 'need'.
We don't absolutely 'need' it. (Configure it out and it's gone).
But when we want to avoid several logging messages coming in from
various CPUs overwriting each other and _not_ introduce additional
latency by locking a single buffer, then yes.
We can possibly reduce it to, say, 1KiB or even lower by imposing
stricter rules on the logging functions.
But I don't see a way around the per-CPU buffer.
Cheers,
Hannes
--
Dr. Hannes ReineckezSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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
Re: [PATCH v2 2/4] scsi: ufs: add debugfs for ufs
2015-02-10 22:58 GMT+09:00 Gilad Broner :
> @@ -5760,6 +5963,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
>
> async_schedule(ufshcd_async_scan, hba);
>
> + UFSDBG_ADD_DEBUGFS(hba);
> +
> return 0;
>
> out_remove_scsi_host:
> @@ -5769,6 +5974,7 @@ exit_gating:
> out_disable:
> hba->is_irq_enabled = false;
> scsi_host_put(host);
> + UFSDBG_REMOVE_DEBUGFS(hba);
> ufshcd_hba_exit(hba);
> out_error:
> return err;
UFSDBG_REMOVE_DEBUGFS() is not called in the driver unloading path.
So ufs debugfs directory is not removed when unloading driver. It
should be called in ufshcd_remove().
BTW, do we really need UFSDBG_ADD_DEBUGFS() and UFSDBG_REMOVE_DEBUGFS()
macros?
We can use static inline functions in ufs-debugfs.h when !CONFIG_DEBUG_FS.
#ifdef CONFIG_DEBUG_FS
void ufsdbg_add_debugfs(struct ufs_hba *hba);
void ufsdbg_remove_debugfs(struct ufs_hba *hba);
#else
static inline void ufsdbg_add_debugfs(struct ufs_hba *hba)
{
}
static inline void ufsdbg_remove_debugfs(struct ufs_hba *hba)
{
}
#endif
--
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
Re: [PATCH] [RESEND] wd719x: add missing .module to wd719x_template
On Mon, 2015-02-09 at 13:38 +0100, Ondrej Zary wrote: > wd719x_template is missing the .module field, causing module refcount > not to work, allowing to rmmod the driver while in use (mounted filesystem), > causing an oops. > > Set .module to THIS_MODULE to fix the problem. This requires a cc to stable, doesn't it? James -- 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
Re: [PATCH] [RESEND] wd719x: add missing .module to wd719x_template
On Thursday 12 February 2015, James Bottomley wrote: > On Mon, 2015-02-09 at 13:38 +0100, Ondrej Zary wrote: > > wd719x_template is missing the .module field, causing module refcount > > not to work, allowing to rmmod the driver while in use (mounted > > filesystem), causing an oops. > > > > Set .module to THIS_MODULE to fix the problem. > > This requires a cc to stable, doesn't it? Should go into 3.19-stable. -- Ondrej Zary -- 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
Re: raid1 narrow_write_error with 4K disks, sd "bad block number requested" messages
On 02/04/2015 11:59 PM, NeilBrown wrote:
On Wed, 28 Jan 2015 10:29:46 -0500 Nate Dailey
wrote:
I'm writing about something that appears to be an issue with raid1's
narrow_write_error, particular to non-512-byte-sector disks. Here's what
I'm doing:
- 2 disk raid1, 4K disks, each connected to a different SAS HBA
- mount a filesystem on the raid1, run a test that writes to it
- remove one of the SAS HBAs (echo 1 >
/sys/bus/pci/devices/\:45\:00.0/remove)
At this point, writes fail and narrow_write_error breaks them up and
retries, one sector at a time. But these are 512-byte sectors, and sd
doesn't like it:
[ 2645.310517] sd 3:0:1:0: [sde] Bad block number requested
[ 2645.310610] sd 3:0:1:0: [sde] Bad block number requested
[ 2645.310690] sd 3:0:1:0: [sde] Bad block number requested
...
There appears to be no real harm done, but there can be a huge number of
these messages in the log.
I can avoid this by disabling bad block tracking, but it looks like
maybe the superblock's bblog_shift is intended to address this exact
issue. However, I don't see a way to change it. Presumably this is
something mdadm should be setting up? I don't see bblog_shift ever set
to anything other than 0.
This is on a RHEL 7.1 kernel, version 3.10.0-221.el7. I took a look at
upstream sd and md changes and nothing jumps out at me that would have
affected this (but I have not tested to see if the bad block messages do
or do not happen on an upstream kernel).
I'd appreciate any advice re: how to handle this. Thanks!
Thanks for the report.
narrow_write_error() should use bdev_logical_block_size() and round up to
that.
Possibly mdadm should get the same information and set bblog_shift
accordingly when creating a bad block log.
I've made a note to fix that, but I'm happy to review patches too :-)
thanks,
NeilBrown
I will post a narrow_write_error patch shortly.
I did some experimentation with setting the bblog_shift in mdadm, but it
didn't work out the way I expected. It turns out that the value is only
loaded from the superblock if:
1453if ((le32_to_cpu(sb->feature_map) & MD_FEATURE_BAD_BLOCKS) &&
1454rdev->badblocks.count == 0) {
...
1473rdev->badblocks.shift = sb->bblog_shift;
And this feature bit is only set if any bad blocks have actually been
recorded.
It also appears to me that the shift is used when loading the bad blocks
from the superblock, but not when storing the bad block list in the
superblock.
Seems like these are bugs, but I'm not certain how the code is supposed
to work (and am getting in a bit over my head with this).
In any case, it doesn't appear to me that there's any harm in having the
bblog_shift not match the disk's block size (right?).
Nate Dailey
--
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
Re: scsi: Implement per-cpu logging buffer
On Thu, Feb 12, 2015 at 02:29:35PM +0100, Hannes Reinecke wrote:
> On 02/12/2015 01:36 PM, Geert Uytterhoeven wrote:
> > On Wed, Feb 11, 2015 at 8:16 PM, Linux Kernel Mailing List
> > wrote:
> >> Gitweb:
> >> http://git.kernel.org/linus/;a=commit;h=ded85c193a391a84076d5c6a7a5668fe164a490e
> >> Commit: ded85c193a391a84076d5c6a7a5668fe164a490e
> >> Parent: b0a93d96b2814c725161f91a4e35d0c29ec0f95b
> >> Refname:refs/heads/master
> >> Author: Hannes Reinecke
> >> AuthorDate: Thu Jan 8 07:43:42 2015 +0100
> >> Committer: Christoph Hellwig
> >> CommitDate: Fri Jan 9 15:44:28 2015 +0100
> >>
> >> scsi: Implement per-cpu logging buffer
> >>
> >> Implement a per-cpu buffer for formatting messages to avoid line breaks
> >> up under high load. This patch implements scmd_printk() and
> >> sdev_prefix_printk() using the per-cpu buffer and makes sdev_printk() a
> >> wrapper for sdev_prefix_printk().
> >>
> >> Tested-by: Robert Elliott
> >> Reviewed-by: Robert Elliott
> >> Signed-off-by: Hannes Reinecke
> >> Signed-off-by: Christoph Hellwig
> >
> >> --- /dev/null
> >> +++ b/drivers/scsi/scsi_logging.c
> >
> >> +#define SCSI_LOG_SPOOLSIZE 4096
> >> +#define SCSI_LOG_BUFSIZE 128
> >> +
> >> +#if (SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE) > BITS_PER_LONG
> >> +#warning SCSI logging bitmask too large
> >> +#endif
> >> +
> >> +struct scsi_log_buf {
> >> + char buffer[SCSI_LOG_SPOOLSIZE];
> >> + unsigned long map;
> >> +};
> >> +
> >> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
> >
> > Do we really need a static 4 KiB per-CPU buffer?
> >
> > bloat-o-meter:
> >
> > add/remove: 183/94 grow/shrink: 314/211 up/down: 33467/-21291 (12176)
> > function old new delta
> > scsi_format_log-4100 +4100
> > handle_mm_fault 17942750+956
> > scsi_log_print_sense_hdr - 774+774
> > proc_keys_show - 770+770
> >
> Define 'need'.
> We don't absolutely 'need' it. (Configure it out and it's gone).
>
> But when we want to avoid several logging messages coming in from
> various CPUs overwriting each other and _not_ introduce additional
> latency by locking a single buffer, then yes.
>
> We can possibly reduce it to, say, 1KiB or even lower by imposing
> stricter rules on the logging functions.
> But I don't see a way around the per-CPU buffer.
It seems very odd to introduce a mechanism like this specifically for
SCSI, rather than introducing a generic per-CPU buffered-print mechanism
in printk, controlled by a config option. That option could then
automatically go away when !SMP, or !PRINTK, or if users don't actually
care about message ordering.
Also, this doesn't seem to be configurable at all.
- Josh Triplett
--
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
Re: [PATCH v2 3/4] scsi: ufs: add trace events and dump prints for debug
On Tue, 10 Feb 2015 15:58:54 +0200
Gilad Broner wrote:
I really hate large patches like this. So much to dig through.
> -#ifdef CONFIG_DEBUG_FS
> +#define UIC_ERR_REG_HIST_LENGTH 8
> +/**
> + * struct ufs_uic_err_reg_hist - keeps history of uic errors
> + * @pos: index to indicate cyclic buffer position
> + * @reg: cyclic buffer for registers value
> + * @tstamp: cyclic buffer for time stamp
> + */
> +struct ufs_uic_err_reg_hist {
> + int pos;
> + u32 reg[UIC_ERR_REG_HIST_LENGTH];
> + ktime_t tstamp[UIC_ERR_REG_HIST_LENGTH];
> +};
> +
> +/**
> + * struct ufs_stats - keeps usage/err statistics
> + * @enabled: enable tagstats for debugfs
> + * @tag_stats: pointer to tag statistic counters
> + * @q_depth: current amount of busy slots
> + * @err_stats: counters to keep track of various errors
> + * @hibern8_exit_cnt: Counter to keep track of number of exits,
> + * reset this after link-startup.
> + * @last_hibern8_exit_tstamp: Set time after the hibern8 exit.
> + * Clear after the first successful command completion.
> + * @pa_err: tracks pa-uic errors
> + * @dl_err: tracks dl-uic errors
> + * @nl_err: tracks nl-uic errors
> + * @tl_err: tracks tl-uic errors
> + * @dme_err: tracks dme errors
> + */
> struct ufs_stats {
> +#ifdef CONFIG_DEBUG_FS
> bool enabled;
> u64 **tag_stats;
> int q_depth;
> int err_stats[UFS_ERR_MAX];
> +#endif
Can any of the tracepoint code be called that references these when
CONFIG_DEBUG_FS is not set? Because soon (possibly even this merge
window), tracing will no longer depend on DEBUG_FS, because it will
have its own filesystem.
-- Steve
--
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
Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes
On Thu, 2015-02-12 at 15:01 -0800, a...@linux-foundation.org wrote: > From: Rasmus Villemoes > Subject: lib/string_helpers.c:string_get_size(): remove redundant prefixes > > While 3c9f3681d0b4 "[SCSI] lib: add generic helper to print sizes rounded > to the correct SI range" says that Z and Y are included in preparation for > 128 bit computers, they just waste .text currently. If and when we get > u128, string_get_size needs updating anyway (and ISO needs to come up with > four more prefixes). This is rubbish. It's nothing to do with 128 bits. This is to do with disk sizes linux gets attached to. The current largest device clusters are Petabytes ... I think we may have some exabyte ones somewhere in the Academic community, so it's by no means inconcievable we'll have Zettabyte ones within a few years. The SCSI standard, with 4k blocks supports up to 2^76, which is well into Zettabytes. We obviously run off the mmap possibilities a lot sooner, because of the byte offsets, but that's fixable. Someone will probably start first by passing blocks into that interface not bytes, so we'd like it not to be based on assumptions that think 2^64 is the largest possible value. > Also there's no need to include and test for the NULL sentinel; once we > reach "E" size is at most 18. [The test is also wrong; it should be > units_str[units][i+1]; if we've reached NULL we're already doomed.] So fix the bug, don't set us up to run off the end of the array. And please consult the community which keeps track of this rather than trying to get it into Linux without review. James -- 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
Re: [PATCH] [SCSI] sg: fix read() error reporting
On 15-02-11 01:13 PM, Tony Battersby wrote:
On 02/11/2015 12:45 PM, Douglas Gilbert wrote:
On 15-02-11 11:32 AM, Tony Battersby wrote:
Fix SCSI generic read() incorrectly returning success after detecting an
error.
Cc:
Signed-off-by: Tony Battersby
---
For inclusion in kernel 3.20.
--- linux-3.19.0/drivers/scsi/sg.c.orig 2015-02-08 21:54:22.0 -0500
+++ linux-3.19.0/drivers/scsi/sg.c 2015-02-10 09:26:09.0 -0500
@@ -546,7 +546,7 @@ static ssize_t
sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
{
sg_io_hdr_t *hp = &srp->header;
- int err = 0;
+ int err = 0, err2;
int len;
if (count < SZ_SG_IO_HDR) {
@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
goto err_out;
}
err_out:
- err = sg_finish_rem_req(srp);
- return (0 == err) ? count : err;
+ err2 = sg_finish_rem_req(srp);
+ return err ? : err2 ? : count;
Tony,
Your point is well made.
I just don't like that last line, using a gcc extension that
hasn't even made it into C11 (or C++11). Wouldn't:
return err ? err : (err2 ? err2 : count);
be a bit better? I think the following snippet makes the intent
clear but would it generate any more code:
if (err || err2)
An improvement on the above line would be:
if (unlikely(err || err2))
return err ? err : err2;
else
return count;
Another approach is to return the good path prior to the
err_out label. This would require two calls to
sg_finish_rem_req().
However Tony's clean-up is sufficient.
I checked before submitting, and the foo ? : bar construct is already
used at least 455 times in the kernel, so it is nothing new.
find linux-3.19 -type f -name "*.[ch]" | xargs grep '\? :' | wc -l
455
But you are probably right; I think that with such small variable names
your version is more readable due to the nesting. Updated patch below.
---
Fix SCSI generic read() incorrectly returning success after detecting an
error.
Cc:
Signed-off-by: Tony Battersby
Acked-by: Douglas Gilbert
---
--- linux-3.19.0/drivers/scsi/sg.c.orig 2015-02-08 21:54:22.0 -0500
+++ linux-3.19.0/drivers/scsi/sg.c 2015-02-11 13:02:56.0 -0500
@@ -546,7 +546,7 @@ static ssize_t
sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
{
sg_io_hdr_t *hp = &srp->header;
- int err = 0;
+ int err = 0, err2;
int len;
if (count < SZ_SG_IO_HDR) {
@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
goto err_out;
}
err_out:
- err = sg_finish_rem_req(srp);
- return (0 == err) ? count : err;
+ err2 = sg_finish_rem_req(srp);
+ return err ? err : (err2 ? err2 : count);
}
static ssize_t
--
--
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
Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes
On Thu, 12 Feb 2015 15:25:08 -0800 James Bottomley wrote: > On Thu, 2015-02-12 at 15:01 -0800, a...@linux-foundation.org wrote: > > From: Rasmus Villemoes > > Subject: lib/string_helpers.c:string_get_size(): remove redundant prefixes > > > > While 3c9f3681d0b4 "[SCSI] lib: add generic helper to print sizes rounded > > to the correct SI range" says that Z and Y are included in preparation for > > 128 bit computers, they just waste .text currently. If and when we get > > u128, string_get_size needs updating anyway (and ISO needs to come up with > > four more prefixes). > > This is rubbish. It's nothing to do with 128 bits. This is to do with > disk sizes linux gets attached to. The current largest device clusters > are Petabytes ... I think we may have some exabyte ones somewhere in the > Academic community, so it's by no means inconcievable we'll have > Zettabyte ones within a few years. The SCSI standard, with 4k blocks > supports up to 2^76, which is well into Zettabytes. We obviously run > off the mmap possibilities a lot sooner, because of the byte offsets, > but that's fixable. Someone will probably start first by passing blocks > into that interface not bytes, so we'd like it not to be based on > assumptions that think 2^64 is the largest possible value. I don't get it. As the man says, this is presently dead code and string_get_size() will need to be changed to work for disks larger than 2^64 bytes. That change may be to take a u128 or it may be as you suggest: replace the `u64 size' with `u64 size, u64 units' which is effectively the same thing. > > Also there's no need to include and test for the NULL sentinel; once we > > reach "E" size is at most 18. [The test is also wrong; it should be > > units_str[units][i+1]; if we've reached NULL we're already doomed.] > > So fix the bug, don't set us up to run off the end of the array. And > please consult the community which keeps track of this rather than > trying to get it into Linux without review. That seems a bit harsh - you've been cc'ed on this every step of the way. -- 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
Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes
On Thu, 2015-02-12 at 15:40 -0800, Andrew Morton wrote: > On Thu, 12 Feb 2015 15:25:08 -0800 James Bottomley > wrote: > > > On Thu, 2015-02-12 at 15:01 -0800, a...@linux-foundation.org wrote: > > > From: Rasmus Villemoes > > > Subject: lib/string_helpers.c:string_get_size(): remove redundant prefixes > > > > > > While 3c9f3681d0b4 "[SCSI] lib: add generic helper to print sizes rounded > > > to the correct SI range" says that Z and Y are included in preparation for > > > 128 bit computers, they just waste .text currently. If and when we get > > > u128, string_get_size needs updating anyway (and ISO needs to come up with > > > four more prefixes). > > > > This is rubbish. It's nothing to do with 128 bits. This is to do with > > disk sizes linux gets attached to. The current largest device clusters > > are Petabytes ... I think we may have some exabyte ones somewhere in the > > Academic community, so it's by no means inconcievable we'll have > > Zettabyte ones within a few years. The SCSI standard, with 4k blocks > > supports up to 2^76, which is well into Zettabytes. We obviously run > > off the mmap possibilities a lot sooner, because of the byte offsets, > > but that's fixable. Someone will probably start first by passing blocks > > into that interface not bytes, so we'd like it not to be based on > > assumptions that think 2^64 is the largest possible value. > > I don't get it. As the man says, this is presently dead code and > string_get_size() will need to be changed to work for disks larger than > 2^64 bytes. That change may be to take a u128 or it may be as you > suggest: replace the `u64 size' with `u64 size, u64 units' which is > effectively the same thing. The first thing someone's going to do is pass in blocks, because that's the way the rest of block functions. If we're lucky the add "ZB" too, but if not we run off the end in some obscure large cluster somewhere. Don't set people up to make mistakes. > > > Also there's no need to include and test for the NULL sentinel; once we > > > reach "E" size is at most 18. [The test is also wrong; it should be > > > units_str[units][i+1]; if we've reached NULL we're already doomed.] > > > > So fix the bug, don't set us up to run off the end of the array. And > > please consult the community which keeps track of this rather than > > trying to get it into Linux without review. > > That seems a bit harsh - you've been cc'ed on this every step of the way. I think you need to check your scripts. This is the first time I've seen this patch, which is why I'm reacting this way. James -- 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
Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes
On Thu, 12 Feb 2015 15:45:29 -0800 James Bottomley wrote: > ... > > > I don't get it. As the man says, this is presently dead code and > > string_get_size() will need to be changed to work for disks larger than > > 2^64 bytes. That change may be to take a u128 or it may be as you > > suggest: replace the `u64 size' with `u64 size, u64 units' which is > > effectively the same thing. > > The first thing someone's going to do is pass in blocks, because that's > the way the rest of block functions. If we're lucky the add "ZB" too, > but if not we run off the end in some obscure large cluster somewhere. > Don't set people up to make mistakes. Well maybe. A little bit. But it assumes that someone is going to make a change then not test it. > > > > Also there's no need to include and test for the NULL sentinel; once we > > > > reach "E" size is at most 18. [The test is also wrong; it should be > > > > units_str[units][i+1]; if we've reached NULL we're already doomed.] > > > > > > So fix the bug, don't set us up to run off the end of the array. And > > > please consult the community which keeps track of this rather than > > > trying to get it into Linux without review. > > > > That seems a bit harsh - you've been cc'ed on this every step of the way. > > I think you need to check your scripts. This is the first time I've > seen this patch, which is why I'm reacting this way. No, james.bottom...@hansenpartnership.com was cc'ed on the original email and on the -mm spam. Perhaps Rasmus should should also have cc'ed linux-scsi - practice seems to vary a lot. But he did cc the scsi maintainer and the author of the patch he was modifying (yourself). So I think the patch is reasonable and the way Rasmus and I handled it is also reasonable. Going nuts at us over it isn't reasonable! -- 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
Re: raid1 narrow_write_error with 4K disks, sd "bad block number requested" messages
On Thu, 12 Feb 2015 11:46:21 -0500 Nate Dailey
wrote:
> On 02/04/2015 11:59 PM, NeilBrown wrote:
> > On Wed, 28 Jan 2015 10:29:46 -0500 Nate Dailey
> > wrote:
> >
> >> I'm writing about something that appears to be an issue with raid1's
> >> narrow_write_error, particular to non-512-byte-sector disks. Here's what
> >> I'm doing:
> >>
> >> - 2 disk raid1, 4K disks, each connected to a different SAS HBA
> >> - mount a filesystem on the raid1, run a test that writes to it
> >> - remove one of the SAS HBAs (echo 1 >
> >> /sys/bus/pci/devices/\:45\:00.0/remove)
> >>
> >> At this point, writes fail and narrow_write_error breaks them up and
> >> retries, one sector at a time. But these are 512-byte sectors, and sd
> >> doesn't like it:
> >>
> >> [ 2645.310517] sd 3:0:1:0: [sde] Bad block number requested
> >> [ 2645.310610] sd 3:0:1:0: [sde] Bad block number requested
> >> [ 2645.310690] sd 3:0:1:0: [sde] Bad block number requested
> >> ...
> >>
> >> There appears to be no real harm done, but there can be a huge number of
> >> these messages in the log.
> >>
> >> I can avoid this by disabling bad block tracking, but it looks like
> >> maybe the superblock's bblog_shift is intended to address this exact
> >> issue. However, I don't see a way to change it. Presumably this is
> >> something mdadm should be setting up? I don't see bblog_shift ever set
> >> to anything other than 0.
> >>
> >> This is on a RHEL 7.1 kernel, version 3.10.0-221.el7. I took a look at
> >> upstream sd and md changes and nothing jumps out at me that would have
> >> affected this (but I have not tested to see if the bad block messages do
> >> or do not happen on an upstream kernel).
> >>
> >> I'd appreciate any advice re: how to handle this. Thanks!
> >
> > Thanks for the report.
> >
> > narrow_write_error() should use bdev_logical_block_size() and round up to
> > that.
> > Possibly mdadm should get the same information and set bblog_shift
> > accordingly when creating a bad block log.
> >
> > I've made a note to fix that, but I'm happy to review patches too :-)
> >
> > thanks,
> > NeilBrown
> >
>
> I will post a narrow_write_error patch shortly.
>
> I did some experimentation with setting the bblog_shift in mdadm, but it
> didn't work out the way I expected. It turns out that the value is only
> loaded from the superblock if:
>
> 1453if ((le32_to_cpu(sb->feature_map) & MD_FEATURE_BAD_BLOCKS) &&
> 1454rdev->badblocks.count == 0) {
> ...
> 1473rdev->badblocks.shift = sb->bblog_shift;
>
> And this feature bit is only set if any bad blocks have actually been
> recorded.
>
> It also appears to me that the shift is used when loading the bad blocks
> from the superblock, but not when storing the bad block list in the
> superblock.
>
> Seems like these are bugs, but I'm not certain how the code is supposed
> to work (and am getting in a bit over my head with this).
Yes, that's probably a bug.
The
} else if (sb->bblog_offset != 0)
rdev->badblocks.shift = 0;
should be
} else if (sb->bblog_offset != 0)
rdev->badblocks.shift = sb->bblog_shift;
>
> In any case, it doesn't appear to me that there's any harm in having the
> bblog_shift not match the disk's block size (right?).
Having the bblog_shift larger than the disk's block size certainly should not
be a problem. Having it small only causes the problem that you have already
discovered.
NeilBrown
>
> Nate Dailey
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
pgpThbUqdUDTs.pgp
Description: OpenPGP digital signature
[Bug 90601] panic on write to 3ware raid array
https://bugzilla.kernel.org/show_bug.cgi?id=90601 Alex Elsayed changed: What|Removed |Added CC||eternaleye+kernelbugs@gmail ||.com --- Comment #9 from Alex Elsayed --- I've been bitten by this as well, on a 3ware 9750. It's currently preventing from using recent kernels on the server in question - which is an issue, because while 3.16.1 is rock-solid for scsi it has an intermittent wireless oops, and 3.17.1 oopses out with this (same backtrace as the existing attachment). -- You are receiving this mail because: You are the assignee for the bug. -- 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
[Bug 90601] panic on write to 3ware raid array
https://bugzilla.kernel.org/show_bug.cgi?id=90601 --- Comment #10 from Alex Elsayed --- Testing with 3.19 shows it's still there for me. -- You are receiving this mail because: You are the assignee for the bug. -- 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
