Re: [PATCH] IB/iser: do I/O path allocations with GFP_NOIO

Re: [PATCH] IB/iser: do I/O path allocations with GFP_NOIO

[Mike Christie]

Should iser patches have linux-scsi ccd on them in the future? And
should they go through the scsi maintainer normally (I understand they
cannot now since James does not have all the infinniband bits)? I am
really just trying to avoid any coordinatation issues that come about by
having core iscsi and tcp iscsi patched sent to the scsi maintainer then
having to have iser going through Roland.

For example I left a bit in the core iscsi code so I would not break
iser. Now iser is updating their code, so we do not need that bit, but
Or's patch missed the cleanup. If we sent everything through one
maintainer then we could have cleaned everything up in one pass.

Does srp go from openib-general and Roland then to lkml? For iscsi we do
not go through net-dev and we live in drivers/scsi so maybe we are the
odd driver?:) What is the proper or normal procedure?





Or Gerlitz wrote:
> Thanks for Mike Christie for pointing this out - Or.
> 
> a block driver is not allowed to use GFP_KERNEL allocations on its I/O code
> path since the allocation might require I/O (eg to pageout other memory),
> resulting in either deadlock or tightloop.
> 
> move I/O path (queuecommand) allocations to be done with GFP_NOIO
> 
> Signed-off-by: Or Gerlitz <ogerlitz@voltaire.com>
> 
> diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
> index 2703bb0..073e7b5 100644
> --- a/drivers/infiniband/ulp/iser/iser_initiator.c
> +++ b/drivers/infiniband/ulp/iser/iser_initiator.c
> @@ -225,7 +225,7 @@ static int iser_post_receive_control(str
>  	struct iser_device  *device = iser_conn->ib_conn->device;
>  	int rx_data_size, err = 0;
> 
> -	rx_desc = kmem_cache_alloc(ig.desc_cache, GFP_KERNEL);
> +	rx_desc = kmem_cache_alloc(ig.desc_cache, GFP_NOIO);
>  	if (rx_desc == NULL) {
>  		iser_err("Failed to alloc desc for post recv\n");
>  		return -ENOMEM;
> @@ -238,7 +238,7 @@ static int iser_post_receive_control(str
>  	else /* FIXME till user space sets conn->max_recv_dlength correctly */
>  		rx_data_size = 128;
> 
> -	rx_desc->data = kmalloc(rx_data_size, GFP_KERNEL);
> +	rx_desc->data = kmalloc(rx_data_size, GFP_NOIO);
>  	if (rx_desc->data == NULL) {
>  		iser_err("Failed to alloc data buf for post recv\n");
>  		err = -ENOMEM;
> @@ -467,7 +467,7 @@ int iser_send_data_out(struct iscsi_conn
>  	iser_dbg("%s itt %d dseg_len %d offset %d\n",
>  		 __func__,(int)itt,(int)data_seg_len,(int)buf_offset);
> 
> -	tx_desc = kmem_cache_alloc(ig.desc_cache, GFP_KERNEL);
> +	tx_desc = kmem_cache_alloc(ig.desc_cache, GFP_NOIO);
>  	if (tx_desc == NULL) {
>  		iser_err("Failed to alloc desc for post dataout\n");
>  		return -ENOMEM;
> diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
> index 0881f55..31950a5 100644
> --- a/drivers/infiniband/ulp/iser/iser_memory.c
> +++ b/drivers/infiniband/ulp/iser/iser_memory.c
> @@ -111,10 +111,10 @@ int iser_start_rdma_unaligned_sg(struct
>  	unsigned long  cmd_data_len = data->data_len;
> 
>  	if (cmd_data_len > ISER_KMALLOC_THRESHOLD)
> -		mem = (void *)__get_free_pages(GFP_KERNEL,
> +		mem = (void *)__get_free_pages(GFP_NOIO,
>  		      long_log2(roundup_pow_of_two(cmd_data_len)) - PAGE_SHIFT);
>  	else
> -		mem = kmalloc(cmd_data_len, GFP_KERNEL);
> +		mem = kmalloc(cmd_data_len, GFP_NOIO);
> 
>  	if (mem == NULL) {
>  		iser_err("Failed to allocate mem size %d %d for copying sglist\n",

Re: [PATCH] IB/iser: do I/O path allocations with GFP_NOIO

[Roland Dreier]

    Mike> Should iser patches have linux-scsi ccd on them in the
    Mike> future? And should they go through the scsi maintainer
    Mike> normally (I understand they cannot now since James does not
    Mike> have all the infinniband bits)? I am really just trying to
    Mike> avoid any coordinatation issues that come about by having
    Mike> core iscsi and tcp iscsi patched sent to the scsi maintainer
    Mike> then having to have iser going through Roland.

    Mike> Does srp go from openib-general and Roland then to lkml? For
    Mike> iscsi we do not go through net-dev and we live in
    Mike> drivers/scsi so maybe we are the odd driver?:) What is the
    Mike> proper or normal procedure?

It's a problem because SRP and iSER are straddling both the SCSI and
IB worlds.  Probably the best policy is to cc all relevant mailing
lists (at least linux-scsi and openib-general) whenever there's a
doubt about who should see something.

As far as merging patches goes, I've been merging SRP changes directly
to Linus, except for generic fixes to <scsi/srp.h>, which I've been
sending through James.  Or felt that iSCSI should be merged through my
tree, but I have no problem if in the future patches bypass my tree.
(But I would like to be cc'ed on changes to IB stuff, especially core
things outside of specific drivers)

(Which all reminds me I have a question about SCSI EH and SRP to send
to the linux-scsi list...)

 - R.

Re: [PATCH] IB/iser: do I/O path allocations with GFP_NOIO

[Or Gerlitz]

Roland Dreier wrote:
> It's a problem because SRP and iSER are straddling both the SCSI and
> IB worlds.  Probably the best policy is to cc all relevant mailing
> lists (at least linux-scsi and openib-general) whenever there's a
> doubt about who should see something.

We are monitoring linux-scsi for tracking iscsi upstream updates.

> As far as merging patches goes, I've been merging SRP changes directly
> to Linus, except for generic fixes to <scsi/srp.h>, which I've been
> sending through James.  Or felt that iSCSI should be merged through my
> tree, but I have no problem if in the future patches bypass my tree.
> (But I would like to be cc'ed on changes to IB stuff, especially core
> things outside of specific drivers)

At this point, we prefer that iser related updates/merges would go 
through Roland, the IB maintainer, if this poses a problem for 
scsi/iscsi updates we are open to send our updates/merges via the scsi 
maintainer.

Or.

Re: [PATCH] IB/iser: do I/O path allocations with GFP_NOIO

[FUJITA Tomonori]

From: Mike Christie <michaelc@cs.wisc.edu>
Subject: Re: [PATCH] IB/iser: do I/O path allocations with GFP_NOIO
Date: Tue, 30 May 2006 14:50:44 -0500

> Should iser patches have linux-scsi ccd on them in the future? And
> should they go through the scsi maintainer normally (I understand they
> cannot now since James does not have all the infinniband bits)? I am
> really just trying to avoid any coordinatation issues that come about by
> having core iscsi and tcp iscsi patched sent to the scsi maintainer then
> having to have iser going through Roland.
> 
> For example I left a bit in the core iscsi code so I would not break
> iser. Now iser is updating their code, so we do not need that bit, but
> Or's patch missed the cleanup. If we sent everything through one
> maintainer then we could have cleaned everything up in one pass.

Roland, as proposed in the past, how about moving the iSER and SRP
drivers to drivers/scsi? As you said, they straddle in SCSI and IB
worlds, however, they are just LLDs like iscsi_tcp, which straddle in
SCSI and TCP worlds.

That would help to avoid coordinatation issues too.

Re: [PATCH] IB/iser: do I/O path allocations with GFP_NOIO

[Roland Dreier]

    > Roland, as proposed in the past, how about moving the iSER
    > and SRP drivers to drivers/scsi? As you said, they
    > straddle in SCSI and IB worlds, however, they are just
    > LLDs like iscsi_tcp, which straddle in SCSI and TCP
    > worlds.

I have no problem with that, although in general I'm not thrilled
about moving files around, since it usually is churn without a
technical advantage.

If SRP and iSER were in drivers/scsi, would that mean James would have
to maintain them?  That would be a change in workflow.

 - R.