subject:"Re\:"


On 3/24/24 16:54, Christoph Hellwig wrote:

Use the SCSI host's dma_alignment field and set it in ->init and remove
the now unused config_scsi_dev method.


Reviewed-by: Bart Van Assche 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/a81db761-7ed2-4e4b-834f-7641f6199fcc%40acm.org.

Re: [PATCH 07/23] scsi: add a dma_alignment field to the host and host template


On 3/24/24 16:54, Christoph Hellwig wrote:

Get drivers out of the business of having to call the block layer
dma alignment limits helpers themselves.


Reviewed-by: Bart Van Assche 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/47028fba-7fb0-4eda-81a2-ccf439cfec6a%40acm.org.

Re: [PATCH 09/23] scsi: use the atomic queue limits API in scsi_add_lun


On 3/24/24 16:54, Christoph Hellwig wrote:

Switch scsi_add_lun to use the atomic queue limits API to update the
max_hw_sectors for devices with quirks.


Reviewed-by: Bart Van Assche 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/bb3b5924-d266-49f5-944f-5e7ee3d3b5b7%40acm.org.

Re: [PATCH 23/23] block: remove now unused queue limits helpers


On 3/24/24 16:54, Christoph Hellwig wrote:

Signed-off-by: Christoph Hellwig 


Reviewed-by: Bart Van Assche 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/d64f697b-7349-4592-814a-00124afb6737%40acm.org.

Re: [PATCH 10/23] scsi: add a device_configure method to the host template


On 3/24/24 16:54, Christoph Hellwig wrote:

This is a version of ->slave_configure that also takes a queue_limits
structure that the caller applies, and thus allows drivers to reconfigure
the queue using the atomic queue limits API.

In the long run it should also replace ->slave_configure entirely as
there is no need to have two different methods here, and the slave
name in addition to being politically charged also has no basis in
the SCSI standards or the kernel code.


There are two methods with names that are politically charged:
slave_configure() and slave_alloc(). Shouldn't both be renamed?


 * Status: OPTIONAL
+*
+* Note: slave_configure is the legacy version, use device_configure for
+* all new code.
 */
+   int (* device_configure)(struct scsi_device *, struct queue_limits 
*lim);
int (* slave_configure)(struct scsi_device *);


The name "device_configure" may make people wonder whether that method
perhaps configures a struct device instance. How about using the name
"sdev_configure" instead of "device_configure" to make it more clear
that this method is used to configure a SCSI device?

Thanks,

Bart.

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/b3ee2dec-3258-4c9f-81d8-0a266128b9ef%40acm.org.

Re: [PATCH 02/23] bsg: pass queue_limits to bsg_setup_queue


On 3/24/24 16:54, Christoph Hellwig wrote:

This allows bsg_setup_queue to pass them to blk_mq_alloc_queue and thus
set up the limits at queue allocation time.


Reviewed-by: Bart Van Assche 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/1159bc83-f252-49fe-a15a-e0d0eb18661f%40acm.org.

Re: [PATCH 04/23] scsi: initialize scsi midlayer limits before allocating the queue


On 3/24/24 16:54, Christoph Hellwig wrote:

Turn __scsi_init_queue into scsi_init_limits which initializes
queue_limits structure that can be passed to blk_mq_alloc_queue.


Reviewed-by: Bart Van Assche 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/26f51e14-0625-4225-aaf0-f4f7bff5c2ba%40acm.org.

Re: [PATCH 23/23] block: remove now unused queue limits helpers

On 3/25/24 08:54, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/bc4293a0-988d-42f3-a94a-b6715d72c204%40kernel.org.

Re: [PATCH 22/23] uas: switch to using ->device_configure to configure queue limits

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_alloc
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Note that uas was the only driver setting these size limits from
> ->slave_alloc and not ->slave_configure and this makes it match
> everyone else.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/43670f3c-b1cd-4732-9e0f-3cfb3ae9233a%40kernel.org.

Re: [PATCH 21/23] mpi3mr: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Note that mpi3mr also updates the limits from an event handler that
> iterates all SCSI devices.  This is also updated to use the
> queue_limits, but the complete locking of this path probably means
> it already is completely broken and needs a proper audit.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/66169fb4-efeb-4da5-8280-a0585537acfd%40kernel.org.

Re: [PATCH 20/23] libata: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me. For the ata bits:

Acked-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/06562b24-1397-4ac3-bb62-f2409503e956%40kernel.org.

Re: [PATCH 19/23] pata_macio: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Acked-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/6944da16-2a6c-42cd-baa1-c6d6d4dfc866%40kernel.org.

Re: [PATCH 18/23] sata_nv: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Acked-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/b1dba4fa-5a5d-443c-aae3-e8e5aea3eafe%40kernel.org.

Re: [PATCH 17/23] usb-storage: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Also use the proper atomic queue limit update helpers and freeze the
> queue when updating max_hw_sectors from sysfs.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/9f2082fd-83d8-4140-84a2-865112090a46%40kernel.org.

Re: [PATCH 16/23] pmcraid: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/2f66396e-fe27-4a0b-a3fc-872bb6f07eee%40kernel.org.

Re: [PATCH 15/23] ipr: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/1890fdc9-41e4-415c-b210-f549e0b8436a%40kernel.org.

Re: [PATCH 14/23] hptiop: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/4f9b30f2-2351-4d58-b368-46c65288aef9%40kernel.org.

Re: [PATCH 13/23] sbp2: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/794533b4-19d7-4682-ad5f-9b19568420c3%40kernel.org.

Re: [PATCH 12/23] mpt3sas: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/b55b31ef-05ca-4487-ae7d-6d107c84f76c%40kernel.org.

Re: [PATCH 11/23] megaraid_sas: switch to using ->device_configure

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch to the ->device_configure method instead of ->slave_configure
> and update the block limits on the passed in queue_limits instead
> of using the per-limit accessors.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/004fec87-af1f-4a6a-aaa7-f57406bead41%40kernel.org.

Re: [PATCH 10/23] scsi: add a device_configure method to the host template

On 3/25/24 08:54, Christoph Hellwig wrote:
> This is a version of ->slave_configure that also takes a queue_limits
> structure that the caller applies, and thus allows drivers to reconfigure
> the queue using the atomic queue limits API.
> 
> In the long run it should also replace ->slave_configure entirely as
> there is no need to have two different methods here, and the slave
> name in addition to being politically charged also has no basis in
> the SCSI standards or the kernel code.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/scsi_scan.c | 33 +++--
>  include/scsi/scsi_host.h |  4 
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 699356d7d17545..8e05780f802523 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -227,7 +227,7 @@ static int scsi_realloc_sdev_budget_map(struct 
> scsi_device *sdev,
>  
>   /*
>* realloc if new shift is calculated, which is caused by setting
> -  * up one new default queue depth after calling ->slave_configure
> +  * up one new default queue depth after calling ->device_configure
>*/
>   if (!need_alloc && new_shift != sdev->budget_map.shift)
>   need_alloc = need_free = true;
> @@ -874,8 +874,9 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
> unsigned char *inq_result,
>  static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>   blist_flags_t *bflags, int async)
>  {
> + const struct scsi_host_template *hostt = sdev->host->hostt;
>   struct queue_limits lim;
> - int ret;
> + int ret, ret2;
>  
>   /*
>* XXX do not save the inquiry, since it can change underneath us,
> @@ -1073,22 +1074,26 @@ static int scsi_add_lun(struct scsi_device *sdev, 
> unsigned char *inq_result,
>   lim.max_hw_sectors = 512;
>   else if (*bflags & BLIST_MAX_1024)
>   lim.max_hw_sectors = 1024;
> - ret = queue_limits_commit_update(sdev->request_queue, &lim);
> +
> + if (hostt->device_configure)
> + ret = hostt->device_configure(sdev, &lim);
> + else if (hostt->slave_configure)
> + ret = hostt->slave_configure(sdev);
> +
> + ret2 = queue_limits_commit_update(sdev->request_queue, &lim);

Why do this if ->device_configure() or ->slave_configure() failed ?
Shouldn't the "if (ret) goto fail" hunk be moved above this call ?

> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index b0948ab69e0fa6..1959193d47e7f5 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -211,7 +211,11 @@ struct scsi_host_template {
>* up after yourself before returning non-0
>*
>* Status: OPTIONAL
> +  *
> +  * Note: slave_configure is the legacy version, use device_configure for
> +  * all new code.

Maybe explictly mention that both *cannot* be defined here ?

>*/
> + int (* device_configure)(struct scsi_device *, struct queue_limits 
> *lim);
>   int (* slave_configure)(struct scsi_device *);
>  
>   /*

With these 2 nits addressed, looks all goo to me. Feel free to add:

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/6199c70e-f0a9-4756-b3fb-106985c41ebf%40kernel.org.

Re: [PATCH 09/23] scsi: use the atomic queue limits API in scsi_add_lun

On 3/25/24 08:54, Christoph Hellwig wrote:
> Switch scsi_add_lun to use the atomic queue limits API to update the
> max_hw_sectors for devices with quirks.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/942dc890-9a1d-4008-944d-816f7a7c470b%40kernel.org.

Re: [PATCH 08/23] ufs-exynos: move setting the the dma alignment to the init method

On 3/25/24 08:54, Christoph Hellwig wrote:
> Use the SCSI host's dma_alignment field and set it in ->init and remove
> the now unused config_scsi_dev method.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/0fa3b0d9-d6a8-4427-80a3-616e54987a77%40kernel.org.

Re: [PATCH 07/23] scsi: add a dma_alignment field to the host and host template

On 3/25/24 08:54, Christoph Hellwig wrote:
> Get drivers out of the business of having to call the block layer
> dma alignment limits helpers themselves.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/3f140e6e-a73b-4c27-a14f-0add8c36dd26%40kernel.org.

Re: [PATCH 06/23] scsi: add a no_highmem flag to struct Scsi_Host

On 3/25/24 08:54, Christoph Hellwig wrote:
> While we really should be killing the block layer bounce buffering ASAP,
> I even more urgently need to stop the drivers to fiddle with the limits
> from ->slave_configure.  Add a no_highmem flag to the Scsi_Host to
> centralize this setting and switch the remaining four drivers that use
> block layer bounce buffering to it.
> 
> Signed-off-by: Christoph Hellwig 

The USB hunks could probably be moved to their own patch following this one ?
But otherwise, looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/80162a6e-12d1-4fd4-ac74-dc5388853323%40kernel.org.

Re: [PATCH 05/23] scsi_transport_fc: add a max_bsg_segments field to struct fc_function_template

On 3/25/24 08:54, Christoph Hellwig wrote:
> ibmvfc only supports a single segment for BSG FC passthrough.  Instead of
> having it set a queue limits after creating the BSD queues, add a field so
> that the FC transport can set it before allocating the queue.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/08451976-7e39-4c7f-8bf5-5eeda4316c4b%40kernel.org.

Re: [PATCH 04/23] scsi: initialize scsi midlayer limits before allocating the queue

On 3/25/24 08:54, Christoph Hellwig wrote:
> Turn __scsi_init_queue into scsi_init_limits which initializes
> queue_limits structure that can be passed to blk_mq_alloc_queue.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/be1655f7-3ae0-4a5a-ac35-95e9c7d2da02%40kernel.org.

Re: [PATCH 03/23] mpi3mr: pass queue_limits to bsg_setup_queue

On 3/25/24 08:54, Christoph Hellwig wrote:
> Pass the limits to bsg_setup_queue instead of setting them up on the live
> queue.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/04beb70f-38b0-46f7-bbbc-24cf40a91d70%40kernel.org.

Re: [PATCH 02/23] bsg: pass queue_limits to bsg_setup_queue

On 3/25/24 08:54, Christoph Hellwig wrote:
> This allows bsg_setup_queue to pass them to blk_mq_alloc_queue and thus
> set up the limits at queue allocation time.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/c9070d06-3095-4bc9-8b9e-ce292404362c%40kernel.org.

Re: [PATCH 01/23] block: don't reject too large max_user_setors in blk_validate_limits

On 3/25/24 08:54, Christoph Hellwig wrote:
> We already cap down the actual max_sectors to the max of the hardware
> and user limit, so don't reject the configuration.
> 
> Signed-off-by: Christoph Hellwig 

Looks OK to me.

Reviewed-by: Damien Le Moal 

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/b8346696-316c-497d-972e-c76d897a1c87%40kernel.org.

Re: [PATCH 2/2] iscsi_tcp: disallow binding the same connection twice

2024-03-19 Thread Mike Christie

On 3/18/24 2:49 PM, Khazhismel Kumykov wrote:
> iscsi_sw_tcp_conn_bind does not check or cleanup previously bound
> sockets, nor should we allow binding the same connection twice.
> 

This looks like a problem for all the iscsi drivers.

I think you could:

1. Add a check for ISCSI_CONN_FLAG_BOUND in iscsi_conn_bind.
2. Have iscsi_sw_tcp_conn_stop do:

/* stop xmit side */
-   iscsi_suspend_tx(conn);
+   iscsi_conn_unbind(cls_conn, true);

to clear the flag when we clean up the conn for relogin.

3. Fix up the other iscsi drivers so they call:

iscsi_conn_unbind(cls_conn, true);

in their failure paths so when they fail they clear ISCSI_CONN_FLAG_BOUND and
iscsi_conn_bind can be called on the retry.



> Signed-off-by: Khazhismel Kumykov 
> ---
>  drivers/scsi/iscsi_tcp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index e8ed60b777c6..8cf5dc203a82 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -716,6 +716,9 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session 
> *cls_session,
>   struct socket *sock;
>   int err;
>  
> + if (tcp_sw_conn->sock)
> + return -EINVAL;
> +
>   /* lookup for existing socket */
>   sock = sockfd_lookup((int)transport_eph, &err);
>   if (!sock) {

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/6bc61553-6c8e-4705-9cbb-8e73d3f8c801%40oracle.com.

Re: [PATCH 1/2] iscsi_tcp: do not bind sockets that already have extra callbacks

2024-03-19 Thread Mike Christie

On 3/18/24 2:49 PM, Khazhismel Kumykov wrote:
> This attempts to avoid a situation where a misbehaving iscsi daemon
> passes a socket for a different iSCSI connection to BIND_CONN - which
> would result in infinite recursion and stack overflow. This will
> also prevent passing *other* sockets which had sk_user_data overridden,
> but that wouldn't have been safe anyways - since we throw away that
> pointer anyways. This does not cover all hypothetical scenarios where we
> pass bad sockets to BIND_CONN.
> 
> This also papers over a different bug - we allow a daemon to call
> BIND_CONN twice for the same connection - which would result in, at the
> least, failing to uninitialize/teardown the previous socket, which will
> be addressed separately.
> 
> Signed-off-by: Khazhismel Kumykov 
> ---
>  drivers/scsi/iscsi_tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 8e14cea15f98..e8ed60b777c6 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -725,7 +725,7 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session 
> *cls_session,
>   }
>  
>   err = -EINVAL;
> - if (!sk_is_tcp(sock->sk))
> + if (!sk_is_tcp(sock->sk) || sock->sk->sk_user_data)
>   goto free_socket;
>  
>   err = iscsi_conn_bind(cls_session, cls_conn, is_leading);


Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/3abd7f0b-dc88-4183-8762-f2f101402edd%40oracle.com.

Re: [PATCH 0/3] drivers: scsi: struct bus_type cleanup

2024-02-12 Thread Martin K. Petersen

On Sat, 03 Feb 2024 15:38:59 -0300, Ricardo B. Marliere wrote:

> This series is part of an effort to cleanup the users of the driver
> core, as can be seen in many recent patches authored by Greg across the
> tree (e.g. [1]). Specifically, this series is part of the task of
> splitting one of his TODOs [2].
> 

Applied to 6.9/scsi-queue, thanks!

[1/3] scsi: fcoe: make fcoe_bus_type const
  https://git.kernel.org/mkp/scsi/c/4dbde797b946
[2/3] scsi: iscsi: make iscsi_flashnode_bus const
  https://git.kernel.org/mkp/scsi/c/824ec98b1b55
[3/3] scsi: scsi_debug: make pseudo_lld_bus const
  https://git.kernel.org/mkp/scsi/c/ac0dd0f33adb

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/170778686840.2103627.12956797195697757629.b4-ty%40oracle.com.

Re: [PATCH 0/3] drivers: scsi: struct bus_type cleanup

2024-02-05 Thread Martin K. Petersen



Ricardo,

> This series is part of an effort to cleanup the users of the driver
> core, as can be seen in many recent patches authored by Greg across
> the tree (e.g. [1]). Specifically, this series is part of the task of
> splitting one of his TODOs [2].

Applied to 6.9/scsi-staging, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/yq1ttmmnyic.fsf%40ca-mkp.ca.oracle.com.

Re: [PATCH 0/3] drivers: scsi: struct bus_type cleanup

2024-02-03 Thread Greg Kroah-Hartman

On Sat, Feb 03, 2024 at 03:38:59PM -0300, Ricardo B. Marliere wrote:
> This series is part of an effort to cleanup the users of the driver
> core, as can be seen in many recent patches authored by Greg across the
> tree (e.g. [1]). Specifically, this series is part of the task of
> splitting one of his TODOs [2].
> 
> ---
> [1]: 
> https://lore.kernel.org/lkml/?q=f%3Agregkh%40linuxfoundation.org+s%3A%22make%22+and+s%3A%22const%22
> [2]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=bus_cleanup&id=26105f537f0c60eacfeb430abd2e05d7ddcdd8aa
> 
> Cc: Greg Kroah-Hartman 
> Signed-off-by: Ricardo B. Marliere 


Reviewed-by: Greg Kroah-Hartman 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/2024020351-groom-underline-d28d%40gregkh.

Re: [PATCH 2/3] scsi: iscsi: make iscsi_flashnode_bus const

2024-02-03 Thread 'Lee Duncan' via open-iscsi

On Sat, Feb 3, 2024 at 10:38 AM Ricardo B. Marliere
 wrote:
>
> Now that the driver core can properly handle constant struct bus_type,
> move the iscsi_flashnode_bus variable to be a constant structure as well,
> placing it into read-only memory which can not be modified at runtime.
>
> Cc: Greg Kroah-Hartman 
> Suggested-by: Greg Kroah-Hartman 
> Signed-off-by: Ricardo B. Marliere 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 3075b2ddf7a6..af3ac6346796 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1201,7 +1201,7 @@ static const struct device_type 
> iscsi_flashnode_conn_dev_type = {
> .release = iscsi_flashnode_conn_release,
>  };
>
> -static struct bus_type iscsi_flashnode_bus;
> +static const struct bus_type iscsi_flashnode_bus;
>
>  int iscsi_flashnode_bus_match(struct device *dev,
>  struct device_driver *drv)
> @@ -1212,7 +1212,7 @@ int iscsi_flashnode_bus_match(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(iscsi_flashnode_bus_match);
>
> -static struct bus_type iscsi_flashnode_bus = {
> +static const struct bus_type iscsi_flashnode_bus = {
> .name = "iscsi_flashnode",
> .match = &iscsi_flashnode_bus_match,
>  };
>
> --
> 2.43.0
>

Reviewed-by: Lee Duncan 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/CAPj3X_UbiLY4MQaUjHkijpSi1LvjG-1BSfO5td7rk__2weshZQ%40mail.gmail.com.

Re: [PATCH] scsi: iscsi: fix iscsi ida memory leak

2024-01-29 Thread Mike Christie

On 1/29/24 3:04 AM, Guixin Liu wrote:
> The iscsi_sess_ida should be destroy when the iscsi module exit.
> 
> Signed-off-by: Guixin Liu 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 3075b2ddf7a6..3c5b42390c47 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -5046,6 +5046,7 @@ static void __exit iscsi_transport_exit(void)
>   class_unregister(&iscsi_endpoint_class);
>   class_unregister(&iscsi_iface_class);
>   class_unregister(&iscsi_transport_class);
> + ida_destroy(&iscsi_sess_ida);
>  }
>  
>  module_init(iscsi_transport_init);

When this is called the ida will be empty so I don't think we have to
call this. From the comments:

/**
 * ida_destroy() - Free all IDs.
 * @ida: IDA handle.
 *
 * Calling this function frees all IDs and releases all resources used
 * by an IDA.  When this call returns, the IDA is empty and can be reused
 * or freed.  If the IDA is already empty, there is no need to call this
 * function.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/9db727d3-1f82-4dd5-ad62-a04d17f6dfed%40oracle.com.

Re: Performance drop due to alloc_workqueue() misuse and recent change

2023-12-19 Thread Tejun Heo

Hello, again.

On Mon, Dec 04, 2023 at 04:03:47PM +, Naohiro Aota wrote:
...
> In summary, we misuse max_active, considering it is a global limit. And,
> the recent commit introduced a huge performance drop in some cases.  We
> need to review alloc_workqueue() usage to check if its max_active setting
> is proper or not.

Can you please test the following branch?

 https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git 
unbound-system-wide-max_active

Thanks.

-- 
tejun

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ZYKUc7MUGvre2lGQ%40slm.duckdns.org.

Re: Performance drop due to alloc_workqueue() misuse and recent change

2023-12-04 Thread Tejun Heo

Hello,

On Mon, Dec 04, 2023 at 04:03:47PM +, Naohiro Aota wrote:
> Recently, commit 636b927eba5b ("workqueue: Make unbound workqueues to use
> per-cpu pool_workqueues") changed WQ_UNBOUND workqueue's behavior. It
> changed the meaning of alloc_workqueue()'s max_active from an upper limit
> imposed per NUMA node to a limit per CPU. As a result, massive number of
> workers can be running at the same time, especially if the workqueue user
> thinks the max_active is a global limit.
> 
> Actually, it is already written it is per-CPU limit in the documentation
> before the commit. However, several callers seem to misuse max_active,
> maybe thinking it is a global limit. It is an unexpected behavior change
> for them.

Right, and the behavior has been like that for a very long time and there
was no other way to achieve reasonable level of concurrency, so the current
situation is expected.

> For example, these callers set max_active = num_online_cpus(), which is a
> suspicious limit applying to per-CPU. This config means we can have nr_cpu
> * nr_cpu active tasks working at the same time.

Yeah, that sounds like a good indicator.

> fs/f2fs/data.c: sbi->post_read_wq = alloc_workqueue("f2fs_post_read_wq",
> fs/f2fs/data.c-  WQ_UNBOUND | 
> WQ_HIGHPRI,
> fs/f2fs/data.c-  num_online_cpus());
> 
> fs/crypto/crypto.c: fscrypt_read_workqueue = 
> alloc_workqueue("fscrypt_read_queue",
> fs/crypto/crypto.c-  WQ_UNBOUND | 
> WQ_HIGHPRI,
> fs/crypto/crypto.c-  
> num_online_cpus());
> 
> fs/verity/verify.c: fsverity_read_workqueue = 
> alloc_workqueue("fsverity_read_queue",
> fs/verity/verify.c-   WQ_HIGHPRI,
> fs/verity/verify.c-   
> num_online_cpus());
> 
> drivers/crypto/hisilicon/qm.c:  qm->wq = alloc_workqueue("%s", WQ_HIGHPRI | 
> WQ_MEM_RECLAIM |
> drivers/crypto/hisilicon/qm.c-   WQ_UNBOUND, 
> num_online_cpus(),
> drivers/crypto/hisilicon/qm.c-   pci_name(qm->pdev));
> 
> block/blk-crypto-fallback.c:blk_crypto_wq = 
> alloc_workqueue("blk_crypto_wq",
> block/blk-crypto-fallback.c-WQ_UNBOUND | 
> WQ_HIGHPRI |
> block/blk-crypto-fallback.c-
> WQ_MEM_RECLAIM, num_online_cpus());
> 
> drivers/md/dm-crypt.c:  cc->crypt_queue = 
> alloc_workqueue("kcryptd/%s",
> drivers/md/dm-crypt.c-
> WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND,
> drivers/md/dm-crypt.c-
> num_online_cpus(), devname);

Most of these work items are CPU bound but not completley so. e.g.
kcrypt_crypt_write_continue() does wait_for_completion(), so setting
max_active to 1 likely isn't what they want either. They mostly want some
reasonable system-wide concurrency limit w.r.t. the CPU count while keeping
some level of flexibility in terms of task placement.

The previous max_active wasn't great for this because its meaning changed
depending on the number of nodes. Now, the meaning doesn't change but it's
not really useful for the above purpose. It's only useful for avoiding
melting the system completely.

One way to go about it is to declare that concurrency level management for
unbound workqueue is on users but that seems not ideal given many use cases
would want it anyway.

Let me think it over but I think the right way to go about it is going the
other direction - ie. making max_active apply to the whole system regardless
of the number of nodes / ccx's / whatever.

> Furthermore, the change affects performance in a certain case.
> 
> Btrfs creates several WQ_UNBOUND workqueues with a default max_active =
> min(NRCPUS + 2, 8). As my machine has 96 CPUs with NUMA disabled, this
> max_active config allows running over 700 active works. Before the commit,
> it is limited to 8 if NUMA is disabled or limited to 16 if NUMA nodes is 2.
> 
> I reverted the workqueue code back to before the commit, and I ran the
> following fio command on RAID0 btrfs on 6 SSDs.
> 
> fio --group_reporting --eta=always --eta-interval=30s --eta-newline=30s \
> --rw=write --fallocate=none \
> --direct=1 --ioengine=libaio --iodepth=32 \
> --filesize=100G \
> --blocksize=64k \
> --time_based --runtime=300s \
> --end_fsync=1 \
> --directory=${MNT} \
> --name=writer --numjobs=32
> 
> By changing workqueue's max_active, the result varies.
> 
> - wq max_active=8   (intended limit by btrfs?)
>   WRITE: bw=2495MiB/s (2616MB/s), 2495MiB/s-2495MiB/s (2616MB/s-2616MB/s), 
> io=753GiB (808GB), run=308953-308953msec
> - wq max_active=16  (actual limit on 2 NUMA nodes setup)
>   WRITE: bw=1736MiB/s (1820MB/s), 1736MiB/s-1736MiB/s (1820MB/s-1820MB/s), 
> io=670GiB (720GB), run=395532-

Re: [PATCH v5 10/10] scsi: scsi_debug: Add param to control sdev's allow_restart

2023-10-09 Thread 'Wenchao Hao' via open-iscsi

On 2023/10/9 7:17, Douglas Gilbert wrote:

On 2023-09-22 05:29, Wenchao Hao wrote:

Add new module param "allow_restart" to control if setup
scsi_device's allow_restart flag. This is used to test scsi
command finished with sense_key 0x6, asc 0x4 and ascq 0x2

Signed-off-by: Wenchao Hao

Hi,
Looked at this and verified that the allow_restart flag of scsi_debug
devices (disks ?) is usually 0 and when the scsi_debug module is
started with allow_restart=1 then the allow_restart flag does indeed
change to 1. For example:
# cat /sys/class/scsi_disk/1\:0\:0\:0/allow_restart
1

That ASC/ASCQ code means: "Logical unit not ready, initializing command
required" according to my library. Played around with sg_start but didn't
see any change in how it reacts. According to scsi_device.h that flag's
description is: "issue START_UNIT in error handler" which implies it
changes how the EH handler reacts.

Perhaps the 3 line patch description could say a little more about how
to use this new parameter...

Sorry I did not write in detail. As you mentioned above, this is to
determine if to trigger error. I would update the commit message to
following lines:

Add new module param "allow_restart" to control if setup scsi_device's
allow_restart flag, this flag determines if trigger EH after command
finished with sense_key 0x6, asc 0x4 and ascq 0x2, EH would be triggered
if allow_restart=1 in this condition.

The new param can be used with error inject added in patch6 to test how
commands finished with sense_key 0x6, asc 0x4 and ascq 0x2 are handled.

Tested-by: Douglas Gilbert

--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/open-iscsi/89365bd3-b4b0-de2d-f863-afbaad118649%40huawei.com.

Re: [PATCH v5 10/10] scsi: scsi_debug: Add param to control sdev's allow_restart

2023-10-08 Thread Douglas Gilbert


On 2023-09-22 05:29, Wenchao Hao wrote:

Add new module param "allow_restart" to control if setup
scsi_device's allow_restart flag. This is used to test scsi
command finished with sense_key 0x6, asc 0x4 and ascq 0x2

Signed-off-by: Wenchao Hao 


Hi,
Looked at this and verified that the allow_restart flag of scsi_debug
devices (disks ?) is usually 0 and when the scsi_debug module is
started with allow_restart=1 then the allow_restart flag does indeed
change to 1. For example:
   # cat /sys/class/scsi_disk/1\:0\:0\:0/allow_restart
   1

That ASC/ASCQ code means: "Logical unit not ready, initializing command
required" according to my library. Played around with sg_start but didn't
see any change in how it reacts. According to scsi_device.h that flag's
description is: "issue START_UNIT in error handler" which implies it
changes how the EH handler reacts.

Perhaps the 3 line patch description could say a little more about how
to use this new parameter...

Tested-by: Douglas Gilbert 






--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/d61e88d3-e1b7-44e0-ba9b-f633be0b5b30%40interlog.com.

Re: [PATCH v5 09/10] scsi: scsi_debug: Add debugfs interface to fail target reset

2023-10-08 Thread Douglas Gilbert


On 2023-09-22 05:29, Wenchao Hao wrote:

The interface is found at
/sys/kernel/debug/scsi_debug/target/fail_reset where 
identifies the target to inject errors on. It's a simple bool type
interface which would make this target's reset fail if set to 'Y'.

Signed-off-by: Wenchao Hao 
Reported-by: kernel test robot 


Tested by setting 'echo 1 > /sys/bus/pseudo/drivers/scsi_debug/opts'
and observing 'tail -f /var/log/syslog'. Looks good including that
fail_reset is readable so its current state can be checked.

Tested-by: Douglas Gilbert 




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/a517343d-cd37-4faa-8c26-c4e0c121%40interlog.com.

Re: [PATCH v5 08/10] scsi: scsi_debug: Add new error injection reset lun failed

2023-10-07 Thread 'Wenchao Hao' via open-iscsi


On 2023/10/7 5:04, Douglas Gilbert wrote:

On 2023-09-22 05:29, Wenchao Hao wrote:

Add error injection type 3 to make scsi_debug_device_reset() return FAILED.
Fail abort command foramt:


s/foramt/format/



Examples:
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0x12" > ${error}


These examples are misleading. Same with the one in patch 7/10 . The example
should be showing an invocation that exercises _this_ patch. So the first
byte of the echo should be 4 not the 0 shown above.

Doug Gilbert



Would update in next version. Would you continue reviewing patch 9/10 and 10/10?

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/fa567e0b-00ca-76ad-f9e7-a554714f813c%40huawei.com.

Re: [PATCH v5 08/10] scsi: scsi_debug: Add new error injection reset lun failed

2023-10-06 Thread Douglas Gilbert


On 2023-09-22 05:29, Wenchao Hao wrote:

Add error injection type 3 to make scsi_debug_device_reset() return FAILED.
Fail abort command foramt:


s/foramt/format/



   ++--+---+
   | Column | Type | Description   |
   ++--+---+
   |   1|  u8  | Error type, fixed to 0x4  |
   ++--+---+
   |   2|  s32 | Error count   |
   ||  |  0: this rule will be ignored |
   ||  |  positive: the rule will always take effect   |
   ||  |  negative: the rule takes effect n times where -n is  |
   ||  |the value given. Ignored after n times |
   ++--+---+
   |   3|  x8  | SCSI command opcode, 0xff for all commands|
   ++--+---+

Examples:
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0x12" > ${error}


These examples are misleading. Same with the one in patch 7/10 . The example
should be showing an invocation that exercises _this_ patch. So the first
byte of the echo should be 4 not the 0 shown above.

Doug Gilbert


will make the device return FAILED when try to reset lun with inquiry
command 10 times.
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0xff" > ${error}
will make the device return FAILED when try to reset lun 10 times.

Usually we do not care about what command it is when trying to perform
reset LUN, so 0xff could be applied.

Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 39 +++
  1 file changed, 39 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 8a16cb9642a6..db8ab6cad078 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -295,6 +295,8 @@ enum sdebug_err_type {
/* with errors set in scsi_cmnd */
ERR_ABORT_CMD_FAILED= 3,/* control return FAILED from */
/* scsi_debug_abort() */
+   ERR_LUN_RESET_FAILED= 4,/* control return FAILED from */
+   /* 
scsi_debug_device_reseLUN_RESET_FAILEDt() */
  };
  
  struct sdebug_err_inject {

@@ -973,6 +975,7 @@ static int sdebug_error_show(struct seq_file *m, void *p)
switch (err->type) {
case ERR_TMOUT_CMD:
case ERR_ABORT_CMD_FAILED:
+   case ERR_LUN_RESET_FAILED:
seq_printf(m, "%d\t%d\t0x%x\n", err->type, err->cnt,
err->cmd);
break;
@@ -1035,6 +1038,7 @@ static ssize_t sdebug_error_write(struct file *file, 
const char __user *ubuf,
switch (inject_type) {
case ERR_TMOUT_CMD:
case ERR_ABORT_CMD_FAILED:
+   case ERR_LUN_RESET_FAILED:
if (sscanf(buf, "%d %d %hhx", &inject->type, &inject->cnt,
   &inject->cmd) != 3)
goto out_error;
@@ -5578,10 +5582,40 @@ static void scsi_debug_stop_all_queued(struct 
scsi_device *sdp)
scsi_debug_stop_all_queued_iter, sdp);
  }
  
+static int sdebug_fail_lun_reset(struct scsi_cmnd *cmnd)

+{
+   struct scsi_device *sdp = cmnd->device;
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+   struct sdebug_err_inject *err;
+   unsigned char *cmd = cmnd->cmnd;
+   int ret = 0;
+
+   if (devip == NULL)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, &devip->inject_err_list, list) {
+   if (err->type == ERR_LUN_RESET_FAILED &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = !!err->cnt;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
  static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)
  {
struct scsi_device *sdp = SCpnt->device;
struct sdebug_dev_info *devip = sdp->hostdata;
+   u8 *cmd = SCpnt->cmnd;
+   u8 opcode = cmd[0];
  
  	++num_dev_resets;
  
@@ -5592,6 +5626,11 @@ static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)

if (devip)
set_bit(SDEBUG_UA_POR, devip->uas_bm);
  
+	if (sdebug_fail_lun_reset(SCpnt)) {

+   scmd_printk(KERN_INFO, SCpnt, "fail lun reset 0x%x\n", opcode);
+   return FAILED;
+   }
+
return SUC

Re: [PATCH v5 07/10] scsi: scsi_debug: Add new error injection abort failed

2023-10-06 Thread Douglas Gilbert


On 2023-09-22 05:29, Wenchao Hao wrote:

Add error injection type 3 to make scsi_debug_abort() return FAILED.
Fail abort command foramt:


s/foramt/format/



   ++--+---+
   | Column | Type | Description   |
   ++--+---+
   |   1|  u8  | Error type, fixed to 0x3  |
   ++--+---+
   |   2|  s32 | Error count   |
   ||  |  0: this rule will be ignored |
   ||  |  positive: the rule will always take effect   |
   ||  |  negative: the rule takes effect n times where -n is  |
   ||  |the value given. Ignored after n times |
   ++--+---+
   |   3|  x8  | SCSI command opcode, 0xff for all commands|
   ++--+---+

Examples:
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0x12" > ${error}
will make the device return FAILED when abort inquiry command 10 times.


Tested with:
   # sg_raw -t 10 -r 1k /dev/sg1 12 00 00 00 60 00

After 10 seconds (the timeout specified to sg_raw) I saw this:
   >>> transport error: Host_status=0x03 [DID_TIME_OUT]

And the
# cat /sys/kernel/debug/scsi_debug/1\:0\:0\:0/error

Count value changed from -10 up to 0 for each invocation of the INQUIRY
command. Thereafter the INQUIRY command worked.

Looks good.

  Tested-by: Douglas Gilbert 



Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 40 +++
  1 file changed, 40 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fe1f7421f617..8a16cb9642a6 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -293,6 +293,8 @@ enum sdebug_err_type {
ERR_FAIL_CMD= 2,/* make specific scsi command's */
/* queuecmd return succeed but */
/* with errors set in scsi_cmnd */
+   ERR_ABORT_CMD_FAILED= 3,/* control return FAILED from */
+   /* scsi_debug_abort() */
  };
  
  struct sdebug_err_inject {

@@ -970,6 +972,7 @@ static int sdebug_error_show(struct seq_file *m, void *p)
list_for_each_entry_rcu(err, &devip->inject_err_list, list) {
switch (err->type) {
case ERR_TMOUT_CMD:
+   case ERR_ABORT_CMD_FAILED:
seq_printf(m, "%d\t%d\t0x%x\n", err->type, err->cnt,
err->cmd);
break;
@@ -1031,6 +1034,7 @@ static ssize_t sdebug_error_write(struct file *file, 
const char __user *ubuf,
  
  	switch (inject_type) {

case ERR_TMOUT_CMD:
+   case ERR_ABORT_CMD_FAILED:
if (sscanf(buf, "%d %d %hhx", &inject->type, &inject->cnt,
   &inject->cmd) != 3)
goto out_error;
@@ -5504,9 +5508,39 @@ static void stop_all_queued(void)
mutex_unlock(&sdebug_host_list_mutex);
  }
  
+static int sdebug_fail_abort(struct scsi_cmnd *cmnd)

+{
+   struct scsi_device *sdp = cmnd->device;
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+   struct sdebug_err_inject *err;
+   unsigned char *cmd = cmnd->cmnd;
+   int ret = 0;
+
+   if (devip == NULL)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, &devip->inject_err_list, list) {
+   if (err->type == ERR_ABORT_CMD_FAILED &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = !!err->cnt;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
  static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
  {
bool ok = scsi_debug_abort_cmnd(SCpnt);
+   u8 *cmd = SCpnt->cmnd;
+   u8 opcode = cmd[0];
  
  	++num_aborts;
  
@@ -5515,6 +5549,12 @@ static int scsi_debug_abort(struct scsi_cmnd *SCpnt)

"%s: command%s found\n", __func__,
ok ? "" : " not");
  
+	if (sdebug_fail_abort(SCpnt)) {

+   scmd_printk(KERN_INFO, SCpnt, "fail abort command 0x%x\n",
+   opcode);
+   return FAILED;
+   }
+
return SUCCESS;
  }
  


--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, sen

Re: [PATCH v5 06/10] scsi: scsi_debug: set command's result and sense data if the error is injected

2023-10-05 Thread Dan Carpenter

Hi Wenchao,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/Wenchao-Hao/scsi-scsi_debug-create-scsi_debug-directory-in-the-debugfs-filesystem/20230922-173226
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:
https://lore.kernel.org/r/20230922092906.2645265-7-haowenchao2%40huawei.com
patch subject: [PATCH v5 06/10] scsi: scsi_debug: set command's result and 
sense data if the error is injected
config: x86_64-randconfig-161-20231003 
(https://download.01.org/0day-ci/archive/20231005/202310050209.vol6gv40-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: 
(https://download.01.org/0day-ci/archive/20231005/202310050209.vol6gv40-...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Reported-by: Dan Carpenter 
| Closes: https://lore.kernel.org/r/202310050209.vol6gv40-...@intel.com/

smatch warnings:
drivers/scsi/scsi_debug.c:7912 scsi_debug_queuecommand() warn: missing error 
code? 'ret'

vim +/ret +7912 drivers/scsi/scsi_debug.c

fd32119b0deac1 Douglas Gilbert 2016-04-25  7832  static int 
scsi_debug_queuecommand(struct Scsi_Host *shost,
fd32119b0deac1 Douglas Gilbert 2016-04-25  7833 
   struct scsi_cmnd *scp)
c2248fc974df7b Douglas Gilbert 2014-11-24  7834  {
c2248fc974df7b Douglas Gilbert 2014-11-24  7835 u8 sdeb_i;
c2248fc974df7b Douglas Gilbert 2014-11-24  7836 struct scsi_device *sdp 
= scp->device;
c2248fc974df7b Douglas Gilbert 2014-11-24  7837 const struct 
opcode_info_t *oip;
c2248fc974df7b Douglas Gilbert 2014-11-24  7838 const struct 
opcode_info_t *r_oip;
c2248fc974df7b Douglas Gilbert 2014-11-24  7839 struct sdebug_dev_info 
*devip;
c2248fc974df7b Douglas Gilbert 2014-11-24  7840 u8 *cmd = scp->cmnd;
c2248fc974df7b Douglas Gilbert 2014-11-24  7841 int (*r_pfp)(struct 
scsi_cmnd *, struct sdebug_dev_info *);
f66b85171a0ebd Martin Wilck2018-02-14  7842 int (*pfp)(struct 
scsi_cmnd *, struct sdebug_dev_info *) = NULL;
c2248fc974df7b Douglas Gilbert 2014-11-24  7843 int k, na;
c2248fc974df7b Douglas Gilbert 2014-11-24  7844 int errsts = 0;
ad0c7775e745d2 Douglas Gilbert 2020-08-21  7845 u64 lun_index = 
sdp->lun & 0x3FFF;
c2248fc974df7b Douglas Gilbert 2014-11-24  7846 u32 flags;
c2248fc974df7b Douglas Gilbert 2014-11-24  7847 u16 sa;
c2248fc974df7b Douglas Gilbert 2014-11-24  7848 u8 opcode = cmd[0];
c2248fc974df7b Douglas Gilbert 2014-11-24  7849 bool has_wlun_rl;
3a90a63d02b8b7 Douglas Gilbert 2020-07-12  7850 bool inject_now;
929aad8ff0578d Wenchao Hao 2023-09-22  7851 int ret = 0;
cc36ffafc0f7e6 Wenchao Hao 2023-09-22  7852 struct 
sdebug_err_inject err;
c2248fc974df7b Douglas Gilbert 2014-11-24  7853  
c2248fc974df7b Douglas Gilbert 2014-11-24  7854 scsi_set_resid(scp, 0);
3a90a63d02b8b7 Douglas Gilbert 2020-07-12  7855 if (sdebug_statistics) {
c483739430f107 Douglas Gilbert 2016-05-06  7856 
atomic_inc(&sdebug_cmnd_count);
3a90a63d02b8b7 Douglas Gilbert 2020-07-12  7857 inject_now = 
inject_on_this_cmd();
3a90a63d02b8b7 Douglas Gilbert 2020-07-12  7858 } else {
3a90a63d02b8b7 Douglas Gilbert 2020-07-12  7859 inject_now = 
false;
3a90a63d02b8b7 Douglas Gilbert 2020-07-12  7860 }
f46eb0e9fc763b Douglas Gilbert 2016-04-25  7861 if 
(unlikely(sdebug_verbose &&
f46eb0e9fc763b Douglas Gilbert 2016-04-25  7862  
!(SDEBUG_OPT_NO_CDB_NOISE & sdebug_opts))) {
c2248fc974df7b Douglas Gilbert 2014-11-24  7863 char b[120];
c2248fc974df7b Douglas Gilbert 2014-11-24  7864 int n, len, sb;
c2248fc974df7b Douglas Gilbert 2014-11-24  7865  
c2248fc974df7b Douglas Gilbert 2014-11-24  7866 len = 
scp->cmd_len;
c2248fc974df7b Douglas Gilbert 2014-11-24  7867 sb = 
(int)sizeof(b);
c2248fc974df7b Douglas Gilbert 2014-11-24  7868 if (len > 32)
c2248fc974df7b Douglas Gilbert 2014-11-24  7869 
strcpy(b, "too long, over 32 bytes");
c2248fc974df7b Douglas Gilbert 2014-11-24  7870 else {
c2248fc974df7b Douglas Gilbert 2014-11-24  7871 for (k 
= 0, n = 0; k < len && n < sb; ++k)
c2248fc974df7b Douglas Gilbert 2014-11-24  7872 
n += scnprintf(b + n, sb - n, "%02x ",
c2248fc974df7b Douglas Gilbert 2014-11-24  7873 
   (u32)cmd[k]);
c2248fc974df7b Douglas Gilbert 2014-11-24  7874 }
458df78b1c513d Bart Van Assche 2018-01-26  7875 
sdev_printk(KERN_INFO, sdp,

Re: [PATCH v5 01/10] scsi: scsi_debug: create scsi_debug directory in the debugfs filesystem

2023-09-27 Thread 'Wenchao Hao' via open-iscsi


On 2023/9/28 9:13, Douglas Gilbert wrote:

On 2023-09-22 05:28, Wenchao Hao wrote:

Create directory scsi_debug in the root of the debugfs filesystem.
Prepare to add interface for manage error injection.

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 9c0af50501f9..35c336271b13 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
@@ -862,6 +863,8 @@ static const int device_qfull_result =
  static const int condition_met_result = SAM_STAT_CONDITION_MET;
+static struct dentry *sdebug_debugfs_root;
+
  /* Only do the extra work involved in logical block provisioning if one or
   * more of the lbpu, lbpws or lbpws10 parameters are given and we are doing
@@ -7011,6 +7014,8 @@ static int __init scsi_debug_init(void)
  goto driver_unreg;
  }
+    sdebug_debugfs_root = debugfs_create_dir("scsi_debug", NULL);


debugfs_create_dir() can fail and return NULL. Looking at other drivers, most
seem to assume it will work. Since the scsi_debug driver is often used to test
abnormal situations, perhaps adding something like:
     if (!sdebug_debugfs_root)
     pr_info("%s: failed to create initial debugfs directory\n", __func__);

might save someone a bit of time if a NULL dereference on sdebug_debugfs_root
follows later. That is what the mpt3sas driver does.



Yes, I would fix it by checking return value of debugfs related call
after your review suggestions for other patches.


Doug Gilbert


+
  for (k = 0; k < hosts_to_add; k++) {
  if (want_store && k == 0) {
  ret = sdebug_add_host_helper(idx);
@@ -7057,6 +7062,7 @@ static void __exit scsi_debug_exit(void)
  sdebug_erase_all_stores(false);
  xa_destroy(per_store_ap);
+    debugfs_remove(sdebug_debugfs_root);
  }
  device_initcall(scsi_debug_init);





--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/382fe161-95fb-3249-32cf-07058f81a4bc%40huawei.com.

Re: [PATCH v5 01/10] scsi: scsi_debug: create scsi_debug directory in the debugfs filesystem

2023-09-27 Thread Douglas Gilbert


On 2023-09-22 05:28, Wenchao Hao wrote:

Create directory scsi_debug in the root of the debugfs filesystem.
Prepare to add interface for manage error injection.

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 9c0af50501f9..35c336271b13 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 
  
@@ -862,6 +863,8 @@ static const int device_qfull_result =
  
  static const int condition_met_result = SAM_STAT_CONDITION_MET;
  
+static struct dentry *sdebug_debugfs_root;

+
  
  /* Only do the extra work involved in logical block provisioning if one or

   * more of the lbpu, lbpws or lbpws10 parameters are given and we are doing
@@ -7011,6 +7014,8 @@ static int __init scsi_debug_init(void)
goto driver_unreg;
}
  
+	sdebug_debugfs_root = debugfs_create_dir("scsi_debug", NULL);


debugfs_create_dir() can fail and return NULL. Looking at other drivers, most
seem to assume it will work. Since the scsi_debug driver is often used to test
abnormal situations, perhaps adding something like:
if (!sdebug_debugfs_root)
pr_info("%s: failed to create initial debugfs directory\n", __func__);

might save someone a bit of time if a NULL dereference on sdebug_debugfs_root
follows later. That is what the mpt3sas driver does.

Doug Gilbert


+
for (k = 0; k < hosts_to_add; k++) {
if (want_store && k == 0) {
ret = sdebug_add_host_helper(idx);
@@ -7057,6 +7062,7 @@ static void __exit scsi_debug_exit(void)
  
  	sdebug_erase_all_stores(false);

xa_destroy(per_store_ap);
+   debugfs_remove(sdebug_debugfs_root);
  }
  
  device_initcall(scsi_debug_init);


--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/8c7cfe09-d145-4387-91cf-da9d4e2398e1%40interlog.com.

Re: [PATCH v5 00/10] scsi:scsi_debug: Add error injection for single device

2023-09-27 Thread Martin K. Petersen



Doug,

> The original error injection mechanism was based on scsi_host which
> could not inject fault for a single SCSI device.
>
> This patchset provides the ability to inject errors for a single SCSI
> device. Now we supports inject timeout errors, queuecommand errors,
> and hostbyte, driverbyte, statusbyte, and sense data for specific SCSI
> Command. Two new error injection is defined to make abort command or
> reset LUN failed.

Please review patches 7 through 10. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/yq1ttrfzkz5.fsf%40ca-mkp.ca.oracle.com.

Re: [PATCH 2/2] scsi: Add comment of target_destroy in scsi_host_template

2023-09-23 Thread 'Wenchao Hao' via open-iscsi


On 2023/9/22 22:53, Bart Van Assche wrote:

On 9/22/23 02:38, Wenchao Hao wrote:

Add comment to tell callback function target_destroy of
scsi_host_template is called in atomic context.

Signed-off-by: Wenchao Hao 
---
  include/scsi/scsi_host.h | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 49f768d0ff37..a72248fa5adf 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -245,6 +245,9 @@ struct scsi_host_template {
   * midlayer calls this point so that the driver may deallocate
   * and terminate any references to the target.
   *
+ * Note: this callback in called with spin_lock held, so donot
+ * call functions might cause schedule
+ *


This comment should mention which spinlock is held.



Would update, thanks for your review suggestion.


Thanks,

Bart.




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/f2fb9e25-c022-58d2-ac56-db35c2edfedf%40huawei.com.

Re: [PATCH 1/2] scsi: core: scsi_device_online() return false if state is SDEV_CANCEL

2023-09-22 Thread Bart Van Assche


On 9/22/23 02:36, Wenchao Hao wrote:

SDEV_CANCEL is set when removing device and scsi_device_online() should
return false if sdev_state is SDEV_CANCEL.

IO hang would be caused if return true when state is SDEV_CANCEL with
following order:

T1: T2:scsi_error_handler
__scsi_remove_device()
   scsi_device_set_state(sdev, SDEV_CANCEL)
scsi_eh_flush_done_q()
if (scsi_device_online(sdev))
  scsi_queue_insert(scmd,...)

The command added by scsi_queue_insert() would never be handled any
more.


Why not? I think the blk_mq_destroy_queue() call in 
__scsi_remove_device() will cause it to fail.


Thanks,

Bart.

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/50b20a3e-e264-4788-8e52-f7b57cf944f0%40acm.org.

Re: [PATCH 2/2] scsi: scsi_error: Fix device reset is not triggered

2023-09-22 Thread Bart Van Assche


On 9/22/23 02:36, Wenchao Hao wrote:

Fix the issue of skipping scsi_try_bus_device_reset() for devices
which is in progress of removing in following order:

T1: T2:scsi_error_handle
__scsi_remove_device
   scsi_device_set_state(sdev, SDEV_DEL)
// would skip device with SDEV_DEL state
shost_for_each_device()
  scsi_try_bus_device_reset
flush all commands
  ...
  scsi_device is released

Some drivers like smartpqi only implement eh_device_reset_handler,
if device reset is skipped, the commands which had been sent to
firmware or devices hardware are not cleared. The error handle
would flush all these commands in scsi_unjam_host().

When the commands are finished by hardware, use after free issue is
triggered.

Add parameter "check_state" to macro shost_for_each_device() to
determine if check device status when traversal scsi_device
of Scsi_Host, and set this parameter to false when traversal
in scsi_error_handle to address this issue.


The above is incomprehensible to me. Please explain more clearly why 
this change is needed.



diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d0911bc28663..db8b9e42267c 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -704,6 +704,23 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
return 0;
  }
  
+static int __scsi_device_get(struct scsi_device *sdev, bool check_state)


"check_state" is a bad argument name because it does not clearly explain 
the purpose of this argument. Would "include_deleted" perhaps be a 
better name?



+{
+   if (check_state &&
+   (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL))
+   goto fail;
+   if (!try_module_get(sdev->host->hostt->module))
+   goto fail;
+   if (!get_device(&sdev->sdev_gendev))
+   goto fail_put_module;
+   return 0;
+
+fail_put_module:
+   module_put(sdev->host->hostt->module);
+fail:
+   return -ENXIO;
+}


Looking at the above code, I think we need two functions: one that does 
not include the sdev->sdev_state check and a second function that 
includes the sdev->sdev_state check (scsi_device_get()) and calls the 
first. That will result in code that is easier to read than calls to a 
function with a boolean argument.



diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c498a12f7715..e166d053c839 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -389,21 +389,25 @@ extern void __starget_for_each_device(struct scsi_target 
*, void *,
  
  /* only exposed to implement shost_for_each_device */

  extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
- struct scsi_device *);
+ struct scsi_device *,
+ bool);
  
  /**

   * shost_for_each_device - iterate over all devices of a host
   * @sdev: the &struct scsi_device to use as a cursor
   * @shost: the &struct scsi_host to iterate over
+ * @check_state: if skip check scsi_device's state to skip some devices
+ *   scsi_device with SDEV_DEL or SDEV_CANCEL would be skipped
+ *   if this is true
   *
   * Iterator that returns each device attached to @shost.  This loop
   * takes a reference on each device and releases it at the end.  If
   * you break out of the loop, you must call scsi_device_put(sdev).
   */
-#define shost_for_each_device(sdev, shost) \
-   for ((sdev) = __scsi_iterate_devices((shost), NULL); \
+#define shost_for_each_device(sdev, shost, check_state) \
+   for ((sdev) = __scsi_iterate_devices((shost), NULL, check_state); \
 (sdev); \
-(sdev) = __scsi_iterate_devices((shost), (sdev)))
+(sdev) = __scsi_iterate_devices((shost), (sdev), check_state))
  
  /**

   * __shost_for_each_device - iterate over all devices of a host (UNLOCKED)


Since only the SCSI error handler passes 0 as 'check_state' argument to 
shost_for_each_device(), instead of adding a boolean argument to that 
macro, please do the following:

* Introduce a new macro for the check_state = 1 case.
* Keep the semantics for shost_for_each_device().

With this approach no SCSI LLDs will have to be modified.

Thanks,

Bart.

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ce9cef41-29e2-4056-a60b-b0e4ee1cc17e%40acm.org.

Re: [PATCH 2/2] scsi: Add comment of target_destroy in scsi_host_template

2023-09-22 Thread Bart Van Assche


On 9/22/23 02:38, Wenchao Hao wrote:

Add comment to tell callback function target_destroy of
scsi_host_template is called in atomic context.

Signed-off-by: Wenchao Hao 
---
  include/scsi/scsi_host.h | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 49f768d0ff37..a72248fa5adf 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -245,6 +245,9 @@ struct scsi_host_template {
 * midlayer calls this point so that the driver may deallocate
 * and terminate any references to the target.
 *
+* Note: this callback in called with spin_lock held, so donot
+* call functions might cause schedule
+*


This comment should mention which spinlock is held.

Thanks,

Bart.

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/9567a78a-baf8-432b-b279-cfc56d370a1d%40acm.org.

Re: [PATCH 1/2] scsi: core: cleanup scsi_dev_queue_ready()

2023-09-22 Thread Damien Le Moal

On 2023/09/22 2:38, Wenchao Hao wrote:
> This is just a cleanup for scsi_dev_queue_ready() to avoid
> redundant goto and if statement, it did not change the origin
> logic.
> 
> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/scsi_lib.c | 35 ++-
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ca5eb058d5c7..f3e388127dbd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1254,28 +1254,29 @@ static inline int scsi_dev_queue_ready(struct 
> request_queue *q,
>   int token;
>  
>   token = sbitmap_get(&sdev->budget_map);
> - if (atomic_read(&sdev->device_blocked)) {
> - if (token < 0)
> - goto out;
> + if (token < 0)
> + return -1;

This is changing how this function works...

>  
> - if (scsi_device_busy(sdev) > 1)
> - goto out_dec;
> + /*
> +  * device_blocked is not set at mostly time, so check it first
> +  * and return token when it is not set.
> +  */
> + if (!atomic_read(&sdev->device_blocked))
> + return token;

...because you reversed the tests order.

>  
> - /*
> -  * unblock after device_blocked iterates to zero
> -  */
> - if (atomic_dec_return(&sdev->device_blocked) > 0)
> - goto out_dec;
> - SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
> -"unblocking device at zero depth\n"));
> + /*
> +  * unblock after device_blocked iterates to zero
> +  */
> + if (scsi_device_busy(sdev) > 1 ||
> + atomic_dec_return(&sdev->device_blocked) > 0) {

And here too, you are changing how the function works. The atomic_dec may not be
done if the first condition is true.

> + sbitmap_put(&sdev->budget_map, token);
> + return -1;
>   }
>  
> + SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
> +  "unblocking device at zero depth\n"));
> +
>   return token;
> -out_dec:
> - if (token >= 0)
> - sbitmap_put(&sdev->budget_map, token);
> -out:
> - return -1;
>  }
>  
>  /*

-- 
Damien Le Moal
Western Digital Research

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ea28de69-8b9d-8ff8-b7fc-eb780123f055%40kernel.org.

Re: [PATCH] scsi: iscsi_tcp: restrict to TCP sockets

2023-09-22 Thread patchwork-bot+netdevbpf

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller :

On Fri, 15 Sep 2023 17:11:11 + you wrote:
> Nothing prevents iscsi_sw_tcp_conn_bind() to receive file descriptor
> pointing to non TCP socket (af_unix for example).
> 
> Return -EINVAL if this is attempted, instead of crashing the kernel.
> 
> Fixes: 7ba247138907 ("[SCSI] open-iscsi/linux-iscsi-5 Initiator: Initiator 
> code")
> Signed-off-by: Eric Dumazet 
> Cc: Lee Duncan 
> Cc: Chris Leech 
> Cc: Mike Christie 
> Cc: "James E.J. Bottomley" 
> Cc: "Martin K. Petersen" 
> Cc: open-iscsi@googlegroups.com
> Cc: linux-s...@vger.kernel.org
> 
> [...]

Here is the summary with links:
  - scsi: iscsi_tcp: restrict to TCP sockets
https://git.kernel.org/netdev/net/c/f4f82c52a0ea

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/169496942315.18728.17720146599552391400.git-patchwork-notify%40kernel.org.

Re: [PATCH] scsi: iscsi_tcp: restrict to TCP sockets

2023-09-15 Thread Mike Christie

On 9/15/23 12:11 PM, Eric Dumazet wrote:
> Nothing prevents iscsi_sw_tcp_conn_bind() to receive file descriptor
> pointing to non TCP socket (af_unix for example).
> 
> Return -EINVAL if this is attempted, instead of crashing the kernel.
> 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ac3eb485-18be-4fcd-b0d4-8370aa64f38a%40oracle.com.

Re: [PATCH v2 1/2] scsi: iscsi: Add length check for nlattr payload

2023-07-31 Thread Martin K. Petersen

On Tue, 25 Jul 2023 10:45:29 +0800, Lin Ma wrote:

> The current NETLINK_ISCSI netlink parsing loop checks every nlmsg to
> make sure the length is bigger than the sizeof(struct iscsi_uevent) and
> then calls iscsi_if_recv_msg(...).
> 
>   nlh = nlmsg_hdr(skb);
>   if (nlh->nlmsg_len < sizeof(*nlh) + sizeof(*ev) ||
> skb->len < nlh->nlmsg_len) {
> break;
>   }
>   ...
>   err = iscsi_if_recv_msg(skb, nlh, &group);
> 
> [...]

Applied to 6.6/scsi-queue, thanks!

[1/2] scsi: iscsi: Add length check for nlattr payload
  https://git.kernel.org/mkp/scsi/c/971dfcb74a80

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/169083266401.2873709.2360198964162460177.b4-ty%40oracle.com.

Re: [PATCH -next] scsi: iscsi: Remove unused extern declaration iscsi_lookup_iface()

2023-07-31 Thread Martin K. Petersen

On Tue, 25 Jul 2023 22:15:31 +0800, YueHaibing wrote:

> This is not used anymore, so can be removed.
> 
> 

Applied to 6.6/scsi-queue, thanks!

[1/1] scsi: iscsi: Remove unused extern declaration iscsi_lookup_iface()
  https://git.kernel.org/mkp/scsi/c/a615e93d6cfe

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/169083266404.2873709.17161054183653404974.b4-ty%40oracle.com.

Re: [PATCH v1 2/2] scsi: iscsi: Add strlen check in iscsi_if_set_{host}_param

2023-07-25 Thread Martin K. Petersen



Lin,

> The function iscsi_if_set_param and iscsi_if_set_host_param converts
> nlattr payload to type char* and then call C string handling functions
> like sscanf and kstrdup.

Applied to 6.6/scsi-staging, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/yq1wmynxwow.fsf%40ca-mkp.ca.oracle.com.

Re: [PATCH v2 1/2] scsi: iscsi: Add length check for nlattr payload

2023-07-25 Thread Martin K. Petersen



Lin,

> The current NETLINK_ISCSI netlink parsing loop checks every nlmsg to
> make sure the length is bigger than the sizeof(struct iscsi_uevent)
> and then calls iscsi_if_recv_msg(...).

Applied to 6.6/scsi-staging, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/yq1351bzba8.fsf%40ca-mkp.ca.oracle.com.

Re: [PATCH -next] scsi: iscsi: Remove unused extern declaration iscsi_lookup_iface()

2023-07-25 Thread Martin K. Petersen



YueHaibing,

> This is not used anymore, so can be removed.

Applied to 6.6/scsi-staging, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/yq18rb3zbb0.fsf%40ca-mkp.ca.oracle.com.

Re: [PATCH -next] scsi: iscsi: Remove unused extern declaration iscsi_lookup_iface()

2023-07-25 Thread Chris Leech

On Tue, Jul 25, 2023 at 10:15:31PM +0800, YueHaibing wrote:
> This is not used anymore, so can be removed.
> 
> Signed-off-by: YueHaibing 

Thanks,

Reviewed-by: Chris Leech 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ZMAGj/yb5XrcPq5d%40rhel-developer-toolbox-latest.

Re: [PATCH v1 2/2] scsi: iscsi: Add strlen check in iscsi_if_set_{host}_param

2023-07-25 Thread Chris Leech

On Tue, Jul 25, 2023 at 10:45:45AM +0800, Lin Ma wrote:
> The function iscsi_if_set_param and iscsi_if_set_host_param converts
> nlattr payload to type char* and then call C string handling functions
> like sscanf and kstrdup.
> 
>   char *data = (char*)ev + sizeof(*ev);
>   ...
>   sscanf(data, "%d", &value);
> 
> However, since the nlattr is provided by the user-space program and
> the nlmsg skb is allocated with GFP_KERNEL instead of GFP_ZERO flag
> (see netlink_alloc_large_skb in netlink_sendmsg), the dirty data
> remained in the heap can cause OOB read for those string handling
> functions.

Reviewed-by: Chris Leech 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ZMAF1%2BP3blgBZ%2B/m%40rhel-developer-toolbox-latest.

Re: [PATCH v2 1/2] scsi: iscsi: Add length check for nlattr payload

2023-07-25 Thread Chris Leech

On Tue, Jul 25, 2023 at 10:45:29AM +0800, Lin Ma wrote:
> The current NETLINK_ISCSI netlink parsing loop checks every nlmsg to
> make sure the length is bigger than the sizeof(struct iscsi_uevent) and
> then calls iscsi_if_recv_msg(...).
> 
>   nlh = nlmsg_hdr(skb);
>   if (nlh->nlmsg_len < sizeof(*nlh) + sizeof(*ev) ||
> skb->len < nlh->nlmsg_len) {
> break;
>   }
>   ...
>   err = iscsi_if_recv_msg(skb, nlh, &group);
> 
> Hence, in iscsi_if_recv_msg, the nlmsg_data can be safely converted to
> iscsi_uevent as the length is already checked.
> 
> However, in the following parsing, the length of nlattr payload is never
> checked before the payload is converted to other data structures in some
> consumers. A bad one for example is function iscsi_set_path(...) who
> converts the payload to type iscsi_path without any checks.

Thank you for doing the code review on this, I think these changes look
good.

Reviewed-by: Chris Leech 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ZMAFoszXQ3vwc9It%40rhel-developer-toolbox-latest.

Re: [PATCH v2] scsi: iscsi: kfree_sensitive() in iscsi_session_free()

2023-07-23 Thread Mike Christie

On 7/19/23 2:45 AM, 杜敏杰 wrote:
> session might contain private part of the password, so better use
> kfree_sensitive() to free it.
> In iscsi_session_free() use kfree_sensitive() to free session->password,
> session->password_in, session->username, session->username_in.
> 
> Signed-off-by: Minjie Du 
> ---
>  drivers/scsi/libiscsi.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 0fda8905e..a307da898 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -3132,10 +3132,10 @@ void iscsi_session_free(struct iscsi_cls_session 
> *cls_session)
>   struct module *owner = cls_session->transport->owner;
>  
>   iscsi_pool_free(&session->cmdpool);
> - kfree(session->password);
> - kfree(session->password_in);
> - kfree(session->username);
> - kfree(session->username_in);
> + kfree_sensitive(session->password);
> + kfree_sensitive(session->password_in);
> + kfree_sensitive(session->username);
> + kfree_sensitive(session->username_in);
>   kfree(session->targetname);
>   kfree(session->targetalias);
>   kfree(session->initiatorname);

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/59d94e6e-27b6-5b12-d592-8f814ee1788f%40oracle.com.

Re: [PATCH v2] scsi: iscsi: kfree_sensitive() in iscsi_session_free()

2023-07-19 Thread Chris Leech

On Wed, Jul 19, 2023 at 07:45:48AM +, 杜敏杰 wrote:
> session might contain private part of the password, so better use
> kfree_sensitive() to free it.
> In iscsi_session_free() use kfree_sensitive() to free session->password,
> session->password_in, session->username, session->username_in.
> 
> Signed-off-by: Minjie Du 

This looks good, thank you for the follow up to Mike's review.

Reviewed-by: Chris Leech 

> ---
>  drivers/scsi/libiscsi.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 0fda8905e..a307da898 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -3132,10 +3132,10 @@ void iscsi_session_free(struct iscsi_cls_session 
> *cls_session)
>   struct module *owner = cls_session->transport->owner;
>  
>   iscsi_pool_free(&session->cmdpool);
> - kfree(session->password);
> - kfree(session->password_in);
> - kfree(session->username);
> - kfree(session->username_in);
> + kfree_sensitive(session->password);
> + kfree_sensitive(session->password_in);
> + kfree_sensitive(session->username);
> + kfree_sensitive(session->username_in);
>   kfree(session->targetname);
>   kfree(session->targetalias);
>   kfree(session->initiatorname);
> -- 
> 2.39.0
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ZLghwZh2jVnLpzim%40rhel-developer-toolbox-latest.

Re: 回复: [PATCH v1] scsi: iscsi: use kfree_sensitive() in iscsi_session_free()

2023-07-19 Thread 'Lee Duncan' via open-iscsi


On 7/18/23 19:28, 杜敏杰 wrote:

Hi Mike!

Thank you for your reply!
Do I need to submit a new patch to kfree_sensitive for 'password_in' and 
'usernames'?



Just submit a V2 version of your original patch, making the changes that 
Mike suggested. You can continue to include my Reviewed-by tag.




regards,
Minjie

-邮件原件-
发件人: Mike Christie 
发送时间: 2023年7月18日 2:26
收件人: 杜敏杰 ; Lee Duncan ; Chris Leech ; James E.J. 
Bottomley ; Martin K. Petersen ; open list:ISCSI 
; open list:ISCSI ; open list 

抄送: opensource.kernel 
主题: Re: [PATCH v1] scsi: iscsi: use kfree_sensitive() in iscsi_session_free()

On 7/17/23 4:26 AM, Minjie Du wrote:

session might contain private part of the password, so better use
kfree_sensitive() to free it.
In iscsi_session_free() use kfree_sensitive() to free session->password.

Signed-off-by: Minjie Du 
---
  drivers/scsi/libiscsi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index
0fda8905e..2f273229c 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3132,7 +3132,7 @@ void iscsi_session_free(struct iscsi_cls_session 
*cls_session)
struct module *owner = cls_session->transport->owner;
  
  	iscsi_pool_free(&session->cmdpool);

-   kfree(session->password);
+   kfree_sensitive(session->password);
kfree(session->password_in);


You then also want kfree_sensitive for password_in.

I would also use it for the usernames then too.


kfree(session->username);
kfree(session->username_in);




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/7483deb3-2b69-37fa-28ea-fb54aac58ff8%40suse.com.

Re: [PATCH v1] scsi: iscsi: use kfree_sensitive() in iscsi_session_free()

2023-07-17 Thread Mike Christie

On 7/17/23 4:26 AM, Minjie Du wrote:
> session might contain private part of the password, so better use
> kfree_sensitive() to free it.
> In iscsi_session_free() use kfree_sensitive() to free session->password.
> 
> Signed-off-by: Minjie Du 
> ---
>  drivers/scsi/libiscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 0fda8905e..2f273229c 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -3132,7 +3132,7 @@ void iscsi_session_free(struct iscsi_cls_session 
> *cls_session)
>   struct module *owner = cls_session->transport->owner;
>  
>   iscsi_pool_free(&session->cmdpool);
> - kfree(session->password);
> + kfree_sensitive(session->password);
>   kfree(session->password_in);

You then also want kfree_sensitive for password_in.

I would also use it for the usernames then too.

>   kfree(session->username);
>   kfree(session->username_in);

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/5bed6236-0db7-37fd-3d0a-4f51874f9c53%40oracle.com.

Re: [PATCH v1] scsi: iscsi: use kfree_sensitive() in iscsi_session_free()

2023-07-17 Thread 'Lee Duncan' via open-iscsi


On 7/17/23 02:26, Minjie Du wrote:

session might contain private part of the password, so better use
kfree_sensitive() to free it.
In iscsi_session_free() use kfree_sensitive() to free session->password.

Signed-off-by: Minjie Du 
---
  drivers/scsi/libiscsi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 0fda8905e..2f273229c 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3132,7 +3132,7 @@ void iscsi_session_free(struct iscsi_cls_session 
*cls_session)
struct module *owner = cls_session->transport->owner;
  
  	iscsi_pool_free(&session->cmdpool);

-   kfree(session->password);
+   kfree_sensitive(session->password);
kfree(session->password_in);
kfree(session->username);
kfree(session->username_in);


Reviewed-by: Lee Duncan 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/cf71648c-eed0-cefb-ddff-1b2f7809dc3b%40suse.com.

Re: [PATCH] scsi: iscsi: fix stop connection cocurrency issue

2023-07-05 Thread michael . christie

On 7/4/23 6:15 AM, lihongweizz wrote:
> From: Rock Li 
> 
> We met a crash issue when iscsi connection was not stable:
> [ 5581.778976]  connection929:0: detected conn error (1020)
> [ 5592.539182] scsi host17: iSCSI Initiator over TCP/IP
> [ 5592.548847]  connection930:0: detected conn error (1020)
> [ 5592.548890] BUG: unable to handle kernel NULL pointer dereference at 
> 0020
> [ 5592.548935] PGD 0
> [ 5592.548947] Oops:  [#1] SNP NOPTI
> [ 5592.548966] CPU: 51 PID: 912890 Comm: kworker/u161:2 Kdump: loaded 
> Tainted: G OE - xx #1
> [ 5592.549022] Hardware name: xx
> [ 5592.549053] Workqueue: iscsi_conn_cleanup 
> scsi_cleanup_conn_work_fn[scsi_transport_iscsi]
> [ 5592.549098] RIP: 0010:iscsi_sw_tcp_release_conn+0x54/0x110[iscsi_tcp]
> [ 5592.549130] Code: fb be 02 00 00 00 48 89 0f e8 88 65 8b c9 48 8b 45 20 f0 
> ff 80 80 00 00 00 0f 88 e3 06 00 00 48 8b 43 08 4c 8b 70 08 49 8b 06 <48> 8b 
> 58 20 4c 8d bb 30 02 00 00 4c 89 ff e8 49 75 as c9 4c 89 ff
> [ 5592.549209] RSP: 0018:ff6937f4283e7e00 EFLAGS: 00010202
> [ 5592.549233] RAX:  RBX: ff347b03a4a4b478 RCX: 
> 
> [ 5592.549265] RDX:  RSI: fe0l RDI: 
> 8a2bc977
> [ 5592.549296] RBP: ff347b063d49d600 R08: ff347b20bffeb878 R09: 
> 03e8
> [ 5592.549327] R10:  R11: ff347b20bffe9b44 R12: 
> ff347b03a4a4b7a8
> [ 5592.549358] R13: ff347b03a4a4e610 R14: ff347b03a4a4b7a8 R15: 
> ff347b03a4a4b068
> [ 5592.549389] FS: () Gs:ff347b20bffc() 
> knlGs:
> [ 5592.549424] CS: 0010 DS:  ES:  CR0: 80050033
> [ 5592.549446] CR2: 0020 CR3: 003a22610005 CR4: 
> 00773ee0
> [ 5592.549469] DR0:  DR1:  DR2: 
> 
> [ 5592.549491] DR3:  DR6: fffe0ff0 DR7: 
> 0400
> [ 5592.549510] PKRU: 5554
> [ 5592.549518] Call Trace:
> [ 5592.549528]  iscsi_sw_tcp_conn_stop+0x5d/0x80 [iscsi_tcp]
> [ 5592.549546]  iscsi_stop_conn+0x66/0xc0 [scsi_transport_iscsi]
> [ 5592.549568]  iscsi_cleanup_conn_workin+0x6e/0xbe [scsi_transport_iscsi]
> 
> After digging the vmcore file, a concurrency scenario was found:
>   
> iscsi_if_rx iscsi_sw_tcp_state_change
>   iscsi_if_recv_msg   iscsi_sw_sk_state_check
> iscsi_if_stop_conn  iscsi_conn_failure
>   cancel_work_sync(iscsi_conn_error_event
> &conn->cleanup_work)
>  queue_work(
>&conn->cleanup_work)
>   iscsi_stop_conn<- Excute cocurrenty -->  iscsi_stop_conn
> 
> iscsi_stop_conn will be excuted cocurrently in different paths.
> Fix this issue by leveraging ep_mutex to protect iscsi_stop_conn.
> 
> Signed-off-by: Rock Li 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index b9b97300e3b3..1da1083509b6 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2307,7 +2307,16 @@ static int iscsi_if_stop_conn(struct iscsi_cls_conn 
> *conn, int flag)
>*/
>   if (flag == STOP_CONN_TERM) {
>   cancel_work_sync(&conn->cleanup_work);
> +
> + /* There is a race window between cancel clean_work and
> +  * connection stopped. Socket down event may queue clean up
> +  * work again before connection stopped which will result
> +  * stop connection cocurrency issue. Avoid this issue
> +  * by leveraging ep_mutex
> +  */
> + mutex_lock(&conn->ep_mutex);
>   iscsi_stop_conn(conn, flag);
> + mutex_unlock(&conn->ep_mutex);
>   } else {
>   /*
>* Figure out if it was the kernel or userspace initiating this.


Thanks for the patch and nice debugging.

The patch will avoid the crash, but if we are calling iscsi_if_stop_conn to 
terminate
the connection (flag == STOP_CONN_TERM) then we don't want to leave the work 
queued
because it will overwrite the state we just set. Later it could cause problems 
because
we should be in a terminated state, but think we are in recovery so we might go 
down
different paths and not clean everything up.

How about the patch below?

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index e527ece12453..960af1cba267 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2290,6 +2290,8 @@ static void iscsi_if_disconnect_bound_ep(struct 
iscsi_cls_conn *conn,
 
 static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
 {
+   bool do_stop;
+
ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
/*
 * For offload, iscsid may not know about t

Re: [PATCH] scsi: iscsi: fix stop connection concurrency issue

2023-07-04 Thread Markus Elfring

…
> After digging the vmcore file, a concurrency scenario was found:
…
>   iscsi_stop_conn<- Excute cocurrenty -->  iscsi_stop_conn
>
> iscsi_stop_conn will be excuted cocurrently in different paths.
…

I hope that some typos (in the referenced subject and change description)
will be avoided for the final commit message.

Regards,
Markus

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/58501a73-f62a-47cd-f478-a80f85d45cbe%40web.de.

Re: [PATCH net-next v5 11/16] scsi: iscsi_tcp: Use sendmsg(MSG_SPLICE_PAGES) rather than sendpage

2023-06-27 Thread Chris Leech

On Fri, Jun 23, 2023 at 11:55:08PM +0100, David Howells wrote:
> Use sendmsg() with MSG_SPLICE_PAGES rather than sendpage.  This allows
> multiple pages and multipage folios to be passed through.
> 
> Signed-off-by: David Howells 
> Reviewed-by: Mike Christie 
> cc: Lee Duncan 
> cc: Chris Leech 
> cc: "James E.J. Bottomley" 
> cc: "Martin K. Petersen" 
> cc: "David S. Miller" 
> cc: Eric Dumazet 
> cc: Jakub Kicinski 
> cc: Paolo Abeni 
> cc: Jens Axboe 
> cc: Matthew Wilcox 
> cc: Al Viro 
> cc: open-iscsi@googlegroups.com
> cc: linux-s...@vger.kernel.org
> cc: target-de...@vger.kernel.org
> cc: net...@vger.kernel.org
> ---
> 
> Notes:
> ver #5)
>  - Split iscsi changes into client and target patches
> 
>  drivers/scsi/iscsi_tcp.c | 26 ++
>  drivers/scsi/iscsi_tcp.h |  2 --
>  2 files changed, 10 insertions(+), 18 deletions(-)

This seems good to me.

Reviewed-by: Chris Leech 
 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 9637d4bc2bc9..9ab8555180a3 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -301,35 +301,32 @@ static int iscsi_sw_tcp_xmit_segment(struct 
> iscsi_tcp_conn *tcp_conn,
>  
>   while (!iscsi_tcp_segment_done(tcp_conn, segment, 0, r)) {
>   struct scatterlist *sg;
> + struct msghdr msg = {};
> + struct bio_vec bv;
>   unsigned int offset, copy;
> - int flags = 0;
>  
>   r = 0;
>   offset = segment->copied;
>   copy = segment->size - offset;
>  
>   if (segment->total_copied + segment->size < segment->total_size)
> - flags |= MSG_MORE | MSG_SENDPAGE_NOTLAST;
> + msg.msg_flags |= MSG_MORE;
>  
>   if (tcp_sw_conn->queue_recv)
> - flags |= MSG_DONTWAIT;
> + msg.msg_flags |= MSG_DONTWAIT;
>  
> - /* Use sendpage if we can; else fall back to sendmsg */
>   if (!segment->data) {
> + if (!tcp_conn->iscsi_conn->datadgst_en)
> + msg.msg_flags |= MSG_SPLICE_PAGES;
>   sg = segment->sg;
>   offset += segment->sg_offset + sg->offset;
> - r = tcp_sw_conn->sendpage(sk, sg_page(sg), offset,
> -   copy, flags);
> + bvec_set_page(&bv, sg_page(sg), copy, offset);
>   } else {
> - struct msghdr msg = { .msg_flags = flags };
> - struct kvec iov = {
> - .iov_base = segment->data + offset,
> - .iov_len = copy
> - };
> -
> - r = kernel_sendmsg(sk, &msg, &iov, 1, copy);
> + bvec_set_virt(&bv, segment->data + offset, copy);
>   }
> + iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bv, 1, copy);
>  
> + r = sock_sendmsg(sk, &msg);
>   if (r < 0) {
>   iscsi_tcp_segment_unmap(segment);
>   return r;
> @@ -746,7 +743,6 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session 
> *cls_session,
>   sock_no_linger(sk);
>  
>   iscsi_sw_tcp_conn_set_callbacks(conn);
> - tcp_sw_conn->sendpage = tcp_sw_conn->sock->ops->sendpage;
>   /*
>* set receive state machine into initial state
>*/
> @@ -777,8 +773,6 @@ static int iscsi_sw_tcp_conn_set_param(struct 
> iscsi_cls_conn *cls_conn,
>   return -ENOTCONN;
>   }
>   iscsi_set_param(cls_conn, param, buf, buflen);
> - tcp_sw_conn->sendpage = conn->datadgst_en ?
> - sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
>   mutex_unlock(&tcp_sw_conn->sock_lock);
>   break;
>   case ISCSI_PARAM_MAX_R2T:
> diff --git a/drivers/scsi/iscsi_tcp.h b/drivers/scsi/iscsi_tcp.h
> index 68e14a344904..89a6fc552f0b 100644
> --- a/drivers/scsi/iscsi_tcp.h
> +++ b/drivers/scsi/iscsi_tcp.h
> @@ -47,8 +47,6 @@ struct iscsi_sw_tcp_conn {
>   /* MIB custom statistics */
>   uint32_tsendpage_failures_cnt;
>   uint32_tdiscontiguous_hdr_cnt;
> -
> - ssize_t (*sendpage)(struct socket *, struct page *, int, size_t, int);
>  };
>  
>  struct iscsi_sw_tcp_host {
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ZJsyUK8DMN%2BP0nQo%40toolbox.

Re: [PATCH net-next v4 11/15] iscsi: Use sendmsg(MSG_SPLICE_PAGES) rather than sendpage

2023-06-23 Thread David Howells

Mike Christie  wrote:

> 
> One question on the target part I had is about the TODO above. Is that
> something you were going to do, or is it something you are asking the target
> people to do?

I've got an in-progress patch for that, but it's not the simplest code to
modify.  I'm holding off on completing it till the simpler cleanup is in.  I
might end up having to push the incomplete patch your way to ask for advice on
how to complete it.

David

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/2611150.1687549556%40warthog.procyon.org.uk.

Re: [PATCH net-next v4 11/15] iscsi: Use sendmsg(MSG_SPLICE_PAGES) rather than sendpage

2023-06-23 Thread Mike Christie

On 6/23/23 6:44 AM, David Howells wrote:
> Use sendmsg() with MSG_SPLICE_PAGES rather than sendpage.  This allows
> multiple pages and multipage folios to be passed through.
> 
> TODO: iscsit_fe_sendpage_sg() should perhaps set up a bio_vec array for the
> entire set of pages it's going to transfer plus two for the header and
> trailer and page fragments to hold the header and trailer - and then call
> sendmsg once for the entire message.
> 
> Signed-off-by: David Howells 
> cc: Lee Duncan 
> cc: Chris Leech 
> cc: Mike Christie 
> cc: Maurizio Lombardi 
> cc: "James E.J. Bottomley" 
> cc: "Martin K. Petersen" 
> cc: "David S. Miller" 
> cc: Eric Dumazet 
> cc: Jakub Kicinski 
> cc: Paolo Abeni 
> cc: Jens Axboe 
> cc: Matthew Wilcox 
> cc: Al Viro 
> cc: open-iscsi@googlegroups.com
> cc: linux-s...@vger.kernel.org
> cc: target-de...@vger.kernel.org
> cc: net...@vger.kernel.org
> ---
> 
> Notes:
> ver #2)
>  - Wrap lines at 80.
> 
>  drivers/scsi/iscsi_tcp.c | 26 +---
>  drivers/scsi/iscsi_tcp.h |  2 --
>  drivers/target/iscsi/iscsi_target_util.c | 15 --

When you send this again can you split it into 2 patches?

drivers/scsi/iscsi_tcp.* is one driver. It's similar to a NFS client and
it has a set of maintainers that you saw in the MAINTAINER file.

drivers/target/iscsi/iscsi_target_util.c is another driver which is similar
to a NFS server. Martin is the overall maintainer but it's a group effort
to review patches.

I've tested and reviewed the iscsi_tcp parts. You can add my:

Reviewed-by: Mike Christie 

For the iscsi_target_util.c part, I'm reviewing it now and hoping Maurizio
and/or Dimitry might review as well.

One question on the target part I had is about the TODO above. Is that
something you were going to do, or is it something you are asking the target
people to do?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/27fd3750-7b9c-9638-26d8-0df3f0e33b81%40oracle.com.

Re: [PATCH 05/11] iscsi: check net namespace for all iscsi lookup

2023-05-12 Thread Mike Christie

On 5/6/23 6:29 PM, Chris Leech wrote:
> @@ -4065,8 +4108,10 @@ iscsi_if_recv_msg(struct net *net, struct sk_buff *skb,
> ev->u.c_session.cmds_max,
> ev->u.c_session.queue_depth);
>   break;
> + /* MARK */

Got an extra comment in there.

>   case ISCSI_UEVENT_CREATE_BOUND_SESSION:
> - ep = iscsi_lookup_endpoint(ev->u.c_bound_session.ep_handle);
> + ep = iscsi_lookup_endpoint(net,
> +ev->u.c_bound_session.ep_handle);

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/1bbea295-69f6-80ad-2000-a48fb1da8eda%40oracle.com.

Re: [PATCH 11/11] iscsi: force destroy sesions when a network namespace exits

On 5/10/23 1:09 PM, michael.chris...@oracle.com wrote:
> On 5/6/23 4:29 PM, Chris Leech wrote:
>> The namespace is gone, so there is no userspace to clean up.
>> Force close all the sessions.
>>
>> This should be enough for software transports, there's no implementation
>> of migrating physical iSCSI hosts between network namespaces currently.
>>
>> Reviewed-by: Hannes Reinecke 
>> Signed-off-by: Chris Leech 
>> ---
>>  drivers/scsi/scsi_transport_iscsi.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>> b/drivers/scsi/scsi_transport_iscsi.c
>> index 15d28186996d..10e9414844d8 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -5235,9 +5235,25 @@ static int __net_init iscsi_net_init(struct net *net)
>>  
>>  static void __net_exit iscsi_net_exit(struct net *net)
>>  {
>> +struct iscsi_cls_session *session, *tmp;
>>  struct iscsi_net *isn;
>> +unsigned long flags;
>> +LIST_HEAD(sessions);
>>  
>>  isn = net_generic(net, iscsi_net_id);
>> +
>> +spin_lock_irqsave(&isn->sesslock, flags);
>> +list_replace_init(&isn->sesslist, &sessions);
>> +spin_unlock_irqrestore(&isn->sesslock, flags);
>> +
>> +/* force session destruction, there is no userspace anymore */
>> +list_for_each_entry_safe(session, tmp, &sessions, sess_list) {
>> +device_for_each_child(&session->dev, NULL,
>> +  iscsi_iter_force_destroy_conn_fn);
>> +flush_work(&session->destroy_work);
> 
> I think if this flush_work actually flushed, then we would be doing a use
> after free because the running work would free the session from under us.
> 
> We should never have a running destroy_work and be ale to see it on the 
> sesslist
> right? Maybe a WARN_ON or something else so it doesn't look like a possible
> bug.

Maybe not a WARN_ON. What happens if there is running destroy_works for this
namespace and we return from this function?

> 
> 
>> +__iscsi_destroy_session(&session->destroy_work);
>> +}
>> +
>>  netlink_kernel_release(isn->nls);
>>  isn->nls = NULL;
>>  }
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/2059dc74-a00b-fd53-f5ce-3dd41bfbf4f1%40oracle.com.

Re: [PATCH 11/11] iscsi: force destroy sesions when a network namespace exits

On 5/6/23 4:29 PM, Chris Leech wrote:
> The namespace is gone, so there is no userspace to clean up.
> Force close all the sessions.
> 
> This should be enough for software transports, there's no implementation
> of migrating physical iSCSI hosts between network namespaces currently.
> 
> Reviewed-by: Hannes Reinecke 
> Signed-off-by: Chris Leech 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 15d28186996d..10e9414844d8 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -5235,9 +5235,25 @@ static int __net_init iscsi_net_init(struct net *net)
>  
>  static void __net_exit iscsi_net_exit(struct net *net)
>  {
> + struct iscsi_cls_session *session, *tmp;
>   struct iscsi_net *isn;
> + unsigned long flags;
> + LIST_HEAD(sessions);
>  
>   isn = net_generic(net, iscsi_net_id);
> +
> + spin_lock_irqsave(&isn->sesslock, flags);
> + list_replace_init(&isn->sesslist, &sessions);
> + spin_unlock_irqrestore(&isn->sesslock, flags);
> +
> + /* force session destruction, there is no userspace anymore */
> + list_for_each_entry_safe(session, tmp, &sessions, sess_list) {
> + device_for_each_child(&session->dev, NULL,
> +   iscsi_iter_force_destroy_conn_fn);
> + flush_work(&session->destroy_work);

I think if this flush_work actually flushed, then we would be doing a use
after free because the running work would free the session from under us.

We should never have a running destroy_work and be ale to see it on the sesslist
right? Maybe a WARN_ON or something else so it doesn't look like a possible
bug.


> + __iscsi_destroy_session(&session->destroy_work);
> + }
> +
>   netlink_kernel_release(isn->nls);
>   isn->nls = NULL;
>  }

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/6ea35c03-09b9-425d-ddcd-8cdbf99f4fe8%40oracle.com.

Re: [PATCH v2 00/11] Make iscsid-kernel communications namespace-aware

On 5/6/23 4:29 PM, Chris Leech wrote:
> Note that with iscsi_tcp, the connected socket will keep the network
> namespace alive after container exit. The namespace will exit once the

What happens for iser? Is it the same?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/02b582ed-8450-4352-d1b5-a68cde26ba80%40oracle.com.

Re: [PATCH 03/11] iscsi: sysfs filtering by network namespace

On 5/6/23 4:29 PM, Chris Leech wrote:
> +#define DECLARE_TRANSPORT_CLASS_NS(cls, nm, su, rm, cfg, ns, nslookup)   
> \
> +struct transport_class cls = {   
> \
> + .class = {  \
> + .name = nm, \
> + .ns_type = ns,  \
> + .namespace = nslookup,  \
> + },  \
> + .setup = su,\
> + .remove = rm,   \
> + .configure = cfg,   \
> +}

I think this would be in the transport class for others to use in the future
and or you want to name it differently so it doesn't clash with the future
transport class naming. Same as other places in the patch.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/3604c700-3106-dfc6-d39b-5583775be029%40oracle.com.

Re: [PATCH 04/11] iscsi: make all iSCSI netlink multicast namespace aware

On 5/6/23 4:29 PM, Chris Leech wrote:
> @@ -2857,11 +2859,17 @@ void iscsi_post_host_event(uint32_t host_no, struct 
> iscsi_transport *transport,
>  enum iscsi_host_event_code code, uint32_t data_size,
>  uint8_t *data)
>  {
> + struct Scsi_Host *shost;
> + struct net *net;
>   struct nlmsghdr *nlh;
>   struct sk_buff *skb;
>   struct iscsi_uevent *ev;
>   int len = nlmsg_total_size(sizeof(*ev) + data_size);
>  
> + shost = scsi_host_lookup(host_no);
> + if (!shost)
> + return;
> +
>   skb = alloc_skb(len, GFP_NOIO);
>   if (!skb) {

Need scsi_host_put. Maybe just grab the net and do the put before the alloc_skb.


>   printk(KERN_ERR "gracefully ignored host event (%d):%d OOM\n",
> @@ -2880,7 +2888,9 @@ void iscsi_post_host_event(uint32_t host_no, struct 
> iscsi_transport *transport,
>   if (data_size)
>   memcpy((char *)ev + sizeof(*ev), data, data_size);
>  
> - iscsi_multicast_skb(skb, ISCSI_NL_GRP_ISCSID, GFP_NOIO);
> + net = iscsi_host_net(shost->shost_data);
> + scsi_host_put(shost);
> + iscsi_multicast_skb(net, skb, ISCSI_NL_GRP_ISCSID, GFP_NOIO);
>  }
>  EXPORT_SYMBOL_GPL(iscsi_post_host_event);
>  
> @@ -2888,11 +2898,17 @@ void iscsi_ping_comp_event(uint32_t host_no, struct 
> iscsi_transport *transport,
>  uint32_t status, uint32_t pid, uint32_t data_size,
>  uint8_t *data)
>  {
> + struct Scsi_Host *shost;
> + struct net *net;
>   struct nlmsghdr *nlh;
>   struct sk_buff *skb;
>   struct iscsi_uevent *ev;
>   int len = nlmsg_total_size(sizeof(*ev) + data_size);
>  
> + shost = scsi_host_lookup(host_no);
> + if (!shost)
> + return;
> +
>   skb = alloc_skb(len, GFP_NOIO);
>   if (!skb) {

Same as above.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/56fad7cf-1bf4-82a1-2acc-53a5fc7f9c70%40oracle.com.

Re: [PATCH 06/11] iscsi: set netns for tcp and iser hosts

On 5/6/23 4:29 PM, Chris Leech wrote:
> @@ -656,6 +646,8 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
>   if (!(ib_dev->attrs.kernel_cap_flags & IBK_SG_GAPS_REG))
>   shost->virt_boundary_mask = SZ_4K - 1;
>  
> + iscsi_host_set_netns(shost, ep->netns);
> +
>   if (iscsi_host_add(shost, ib_dev->dev.parent)) {
>   mutex_unlock(&iser_conn->state_mutex);
>   goto free_host;
> @@ -663,6 +655,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
>   mutex_unlock(&iser_conn->state_mutex);
>   } else {
>   shost->can_queue = min_t(u16, cmds_max, ISER_DEF_XMIT_CMDS_MAX);
> + iscsi_host_set_netns(shost, net);

Just a nit, but use a consistent coding style. Do you like newlines before/after
like above or not like here.

>   if (iscsi_host_add(shost, NULL))
>   goto free_host;
>   }
> @@ -694,6 +687,34 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
>   return NULL;
>  }
>  
> +/**
> + * iscsi_iser_session_create() - create an iscsi-iser session
> + * @ep: iscsi end-point handle
> + * @cmds_max:   maximum commands in this session
> + * @qdepth: session command queue depth
> + * @initial_cmdsn:  initiator command sequnce number
> + *
> + * Allocates and adds a scsi host, expose DIF support if
> + * exists, and sets up an iscsi session.

Maybe you want these comments for __iscsi_iser_session_create so you can
describe the ep vs a net args. Or it's just odd to have it for only the
ep function.


> @@ -3106,14 +3128,21 @@ static int
>  iscsi_if_create_session(struct iscsi_internal *priv, struct iscsi_endpoint 
> *ep,
>   struct iscsi_uevent *ev, pid_t pid,
>   uint32_t initial_cmdsn, uint16_t cmds_max,
> - uint16_t queue_depth)
> + uint16_t queue_depth, struct net *net)
>  {
>   struct iscsi_transport *transport = priv->iscsi_transport;
>   struct iscsi_cls_session *session;
>   struct Scsi_Host *shost;
>  
> - session = transport->create_session(ep, cmds_max, queue_depth,
> - initial_cmdsn);
> + if (ep) {
> + session = transport->create_session(ep, cmds_max, queue_depth,
> + initial_cmdsn);
> + } else {
> + session = transport->create_session_net(net, cmds_max,
> + queue_depth,
> + initial_cmdsn);
> + }


No {}s are neede here and above. It will also match the other code you
adding.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/29152592-ac41-546d-aeb6-867c6de2c860%40oracle.com.

Re: [PATCH 07/11] iscsi: convert flashnode devices from bus to class

On 5/6/23 4:29 PM, Chris Leech wrote:
> From: Lee Duncan 
> 
> The flashnode session and connection devices should be filtered by net
> namespace along with the iscsi_host, but we can't do that with a bus
> device.  As these don't use any of the bus matching functionality, they
> make more sense as a class device anyway.
> 

Will offload always use the default namespace? If so, there's no need to
touch this code right?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/2a771482-b582-246f-6921-afa1832d7dc0%40oracle.com.

Re: [PATCH 10/11] iscsi: make session and connection lists per-net