| Message ID | 1461013588-8825-1-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Thanks Christoph,
Acked-by: Sagi Grimberg <sagi@grimberg.me>
Doug, this needs a stable tag.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/19/2016 12:06 AM, Christoph Hellwig wrote: > iSER currently has a couple places that set max_sectors in either the host > template or SCSI host, and all of them get it wrong. > > This patch instead uses a single assignment that (hopefully) gets it right: > the max_sectors value must be derived from the number of segments in the > FR or FRM structure, but actually be one lower than the page size multiplied * FRM ==> FMR > by the number of sectors, as it has to handle the case of non-aligned I/O. > > Without this I get trivivial to reproduce hangs when running xfstests > (on XFS) over iSER to Linux targets. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > Looks good. Reviewed-by: Max Gurtovoy <maxg@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 19, 2016 at 12:06 AM, Christoph Hellwig <hch@lst.de> wrote: > iSER currently has a couple places that set max_sectors in either the host > template or SCSI host, and all of them get it wrong. > > This patch instead uses a single assignment that (hopefully) gets it right: > the max_sectors value must be derived from the number of segments in the > FR or FRM structure, but actually be one lower than the page size multiplied > by the number of sectors, as it has to handle the case of non-aligned I/O. > > Without this I get trivivial to reproduce hangs when running xfstests > (on XFS) over iSER to Linux targets. nit, trivivial --> trivial, please fix Sagi, Christoph, You mentioned stable kernels, can you spot the breakage point? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 19, 2016 at 12:06 AM, Christoph Hellwig <hch@lst.de> wrote: > iSER currently has a couple places that set max_sectors in either the host > template or SCSI host, and all of them get it wrong. > > This patch instead uses a single assignment that (hopefully) gets it right: > the max_sectors value must be derived from the number of segments in the > FR or FRM structure, but actually be one lower than the page size multiplied > by the number of sectors, as it has to handle the case of non-aligned I/O. > > Without this I get trivivial to reproduce hangs when running xfstests > (on XFS) over iSER to Linux targets. nit but please fix IB/iser: fix max_sectors calculation --> IB/iser: Fix calculation of maximal number of sectors posted to SCSI layer -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I haven't used that explicit language, but: go fuck yourself, seriously. If you as a maintained want to update changelogs to fit your taste go for it (I do it all the time), but sending half a dozen mails to fix minor changelog formatting for a stable patch that fixes a regression just makes me angry. I thus withdraw this patch and officially don't care about the broken state of iSER anymore. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 10:55 AM, Christoph Hellwig <hch@lst.de> wrote: > I haven't used that explicit language, but: go fuck yourself, seriously. Christoph, I wrote you on both cases this was a nit comment. I understand now that you are not willing to accept nits as feedback on your fixes, and that you are really nervous on me, happens. I suggest you seek (and hopefully find) other means to get relaxed, I know you are top kernel contributor but this doesn't justify talking like this to me nor to anyone else, nor refusing to fix nits if asked by reviewers. > If you as a maintained want to update changelogs to fit your taste go > for it (I do it all the time), but sending half a dozen mails to fix > minor changelog formatting for a stable patch that fixes a regression > just makes me angry. If you prefer it this way, sure, I can do that. > I thus withdraw this patch and officially don't care about the broken > state of iSER anymore. Slow down. As I said, taking into account the volume of your work on the driver, if you prefer nits to be fixed by others we can do that. Or. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
OK let's calm down... Christoph, no need to get angry, Or as a reviewer can suggest a change-log fix, and you as a contributor can simply ignore it if you don't want to fix it, no reason for the harsh language. As you said, Or can fix it if he really cares about it. Or, I personally don't care that much about format/capitalization all that much, but I know you do, so it's fine if we conform to that, but we can fix it ourselves and ease up on the nit-picking in this area. Thank you both, for contributing and reviewing! Cheers, Switzerland. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 2:15 PM, Sagi Grimberg <sagi@grimberg.me> wrote: > OK let's calm down... [...] > Or, I personally don't care that much about format/capitalization all > that much, but I know you do, so it's fine if we conform to that, but > we can fix it ourselves and ease up on the nit-picking in this area. Makes sense to me, want me to pick this up here or you gonna? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 01:51:21PM +0300, Or Gerlitz wrote: > On Thu, Apr 28, 2016 at 10:55 AM, Christoph Hellwig <hch@lst.de> wrote: > > I haven't used that explicit language, but: go fuck yourself, seriously. > > Christoph, I wrote you on both cases this was a nit comment. > > I understand now that you are not willing to accept nits as feedback on > your fixes, and that you are really nervous on me, happens. I wondered when this would finally blow up.. I would also prefer the nit picking on commit language to stop. It is not a welcoming environment for new contributors, and it is a pain to those who are doing the bulk of the work. Get Doug to fix stuff up to match your preferences or let it go. It is *really* not OK to ask Sagi to not ack such commits when the code is OK. Sagi is doing a fine job. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 7:28 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Thu, Apr 28, 2016 at 01:51:21PM +0300, Or Gerlitz wrote: >> On Thu, Apr 28, 2016 at 10:55 AM, Christoph Hellwig <hch@lst.de> wrote: >> > I haven't used that explicit language, but: go fuck yourself, seriously. >> >> Christoph, I wrote you on both cases this was a nit comment. >> >> I understand now that you are not willing to accept nits as feedback on >> your fixes, and that you are really nervous on me, happens. > > I wondered when this would finally blow up.. > I would also prefer the nit picking on commit language to stop. It is > not a welcoming environment for new contributors, and it is a pain to > those who are doing the bulk of the work. So you are a native English speaker that prefers to have commit change-logs with spelling mistakes vs. see a nit reviewer comment followed by a respin by the submitter? your taste, I don't think this is the way to go, the change-log stays there for ever and there's no reason on earth not to fix spelling if we can do that throughout the review. And, yes, I do think that new comers and old suffers need to run spelling on their change-logs before submitting. > Get Doug to fix stuff up to match your preferences or let it go. How about you get Doug to merge for-next code prior to ten days into the merge window? we are 10K KM from having that. > It is *really* not OK to ask Sagi to not ack such commits when the > code is OK. Sagi is doing a fine job. I disagree. We have some conventions for that driver commits, and we have the right to keep them. If Christoph prefers that we will do the final touch, sure, we can do that. And hey, thanks for writing that without cursing [1] me. Or. [1] being a non native English speaker, I wasn't sure if that the right word, I mean, not telling go fuck yourself or alike on every 2nd sentence emitted to the world -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 11:21:45PM +0300, Or Gerlitz wrote: > > I would also prefer the nit picking on commit language to stop. It is > > not a welcoming environment for new contributors, and it is a pain to > > those who are doing the bulk of the work. > > So you are a native English speaker that prefers to have commit > change-logs with spelling mistakes vs. see a nit reviewer comment > followed by a respin by the submitter? What I do care about is growing our community and drawing in more high quality contributions, so seeing good work derided with a bunch of ugly emails is far, far worse in my mind. I can read past a few minor capitalization, spelling and grammar errors in a commit, and will happily accept that to improve our community. As a native speaker, I can tell you that the tone of the last batch of emails you sent was very poor. Too accusatory and demanding for the level of nitpicking you were doing. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Or, I personally don't care that much about format/capitalization all >> that much, but I know you do, so it's fine if we conform to that, but >> we can fix it ourselves and ease up on the nit-picking in this area. > > Makes sense to me, want me to pick this up here or you gonna? I'll take it with Doug, no problem. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 80b6bed..64b3d11 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -612,6 +612,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, struct Scsi_Host *shost; struct iser_conn *iser_conn = NULL; struct ib_conn *ib_conn; + u32 max_fr_sectors; u16 max_cmds; shost = iscsi_host_alloc(&iscsi_iser_sht, 0, 0); @@ -632,7 +633,6 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, iser_conn = ep->dd_data; max_cmds = iser_conn->max_cmds; shost->sg_tablesize = iser_conn->scsi_sg_tablesize; - shost->max_sectors = iser_conn->scsi_max_sectors; mutex_lock(&iser_conn->state_mutex); if (iser_conn->state != ISER_CONN_UP) { @@ -657,8 +657,6 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, */ shost->sg_tablesize = min_t(unsigned short, shost->sg_tablesize, ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len); - shost->max_sectors = min_t(unsigned int, - 1024, (shost->sg_tablesize * PAGE_SIZE) >> 9); if (iscsi_host_add(shost, ib_conn->device->ib_device->dma_device)) { @@ -672,6 +670,15 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, goto free_host; } + /* + * FRs or FMRs can only map up to a (device) page per entry, but if the + * first entry is misaligned we'll end up using using two entries + * (head and tail) for a single page worth data, so we have to drop + * one segment from the calculation. + */ + max_fr_sectors = ((shost->sg_tablesize - 1) * PAGE_SIZE) >> 9; + shost->max_sectors = min(iser_max_sectors, max_fr_sectors); + if (cmds_max > max_cmds) { iser_info("cmds_max changed from %u to %u\n", cmds_max, max_cmds); @@ -989,7 +996,6 @@ static struct scsi_host_template iscsi_iser_sht = { .queuecommand = iscsi_queuecommand, .change_queue_depth = scsi_change_queue_depth, .sg_tablesize = ISCSI_ISER_DEF_SG_TABLESIZE, - .max_sectors = ISER_DEF_MAX_SECTORS, .cmd_per_lun = ISER_DEF_CMD_PER_LUN, .eh_abort_handler = iscsi_eh_abort, .eh_device_reset_handler= iscsi_eh_device_reset,
iSER currently has a couple places that set max_sectors in either the host template or SCSI host, and all of them get it wrong. This patch instead uses a single assignment that (hopefully) gets it right: the max_sectors value must be derived from the number of segments in the FR or FRM structure, but actually be one lower than the page size multiplied by the number of sectors, as it has to handle the case of non-aligned I/O. Without this I get trivivial to reproduce hangs when running xfstests (on XFS) over iSER to Linux targets. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)