2015-11-12 16:31:55

by Lee Duncan

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On 10/14/2015 08:53 PM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this? We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
>
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
>
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
>
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective. Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X. With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event. If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
>
> James

James?

It looks like both Hannes and GregKH agreed this was the proper approach.

--
Lee Duncan


2015-11-13 21:55:23

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

>>>>> "Lee" == Lee Duncan <[email protected]> writes:

>> Well, I'm a bit worried about the loss of a monotonically increasing
>> host number from the debugging perspective. Right now, if you look
>> at any log, hostX always refers to one and only one incarnation
>> throughout the system lifetime for any given value of X.

That's a feature that I would absolutely hate to lose. I spend a huge
amount of time looking at system logs.

--
Martin K. Petersen Oracle Linux Engineering

2015-11-16 12:10:11

by Hannes Reinecke

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On 11/13/2015 10:54 PM, Martin K. Petersen wrote:
>>>>>> "Lee" == Lee Duncan <[email protected]> writes:
>
>>> Well, I'm a bit worried about the loss of a monotonically increasing
>>> host number from the debugging perspective. Right now, if you look
>>> at any log, hostX always refers to one and only one incarnation
>>> throughout the system lifetime for any given value of X.
>
> That's a feature that I would absolutely hate to lose. I spend a huge
> amount of time looking at system logs.
>
Right. Then have it enabled via a modprobe parameters.

We actually had customers running into a host_no overflow due to
excessive host allocations and freeing done by iSCSI.

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

2015-11-16 21:47:44

by Lee Duncan

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On 11/16/2015 04:10 AM, Hannes Reinecke wrote:
> On 11/13/2015 10:54 PM, Martin K. Petersen wrote:
>>>>>>> "Lee" == Lee Duncan <[email protected]> writes:
>>
>>>> Well, I'm a bit worried about the loss of a monotonically increasing
>>>> host number from the debugging perspective. Right now, if you look
>>>> at any log, hostX always refers to one and only one incarnation
>>>> throughout the system lifetime for any given value of X.
>>
>> That's a feature that I would absolutely hate to lose. I spend a huge
>> amount of time looking at system logs.
>>
> Right. Then have it enabled via a modprobe parameters.
>
> We actually had customers running into a host_no overflow due to
> excessive host allocations and freeing done by iSCSI.
>
> Cheers,
>
> Hannes
>

Martin: I will be glad to update the patch, creating a modprobe
parameter as suggested, if you find this acceptable.
--
Lee Duncan
SUSE Labs

2015-11-17 23:21:17

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

>>>>> "Lee" == Lee Duncan <[email protected]> writes:

Lee> Martin: I will be glad to update the patch, creating a modprobe
Lee> parameter as suggested, if you find this acceptable.

For development use a module parameter would be fine. But I am concerned
about our support folks that rely on the incrementing host number when
analyzing customer log files.

Ewan: How do you folks feel about this change?

--
Martin K. Petersen Oracle Linux Engineering

2015-12-10 21:49:28

by Lee Duncan

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>> "Lee" == Lee Duncan <[email protected]> writes:
>
> Lee> Martin: I will be glad to update the patch, creating a modprobe
> Lee> parameter as suggested, if you find this acceptable.
>
> For development use a module parameter would be fine. But I am concerned
> about our support folks that rely on the incrementing host number when
> analyzing customer log files.
>
> Ewan: How do you folks feel about this change?
>

Ewan?

--
Lee Duncan

2015-12-11 15:31:55

by Ewan Milne

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
> >>>>>> "Lee" == Lee Duncan <[email protected]> writes:
> >
> > Lee> Martin: I will be glad to update the patch, creating a modprobe
> > Lee> parameter as suggested, if you find this acceptable.
> >
> > For development use a module parameter would be fine. But I am concerned
> > about our support folks that rely on the incrementing host number when
> > analyzing customer log files.
> >
> > Ewan: How do you folks feel about this change?
> >
>
> Ewan?


Personally, I think having host numbers that increase essentially
without limit (I think I've seen this with iSCSI sessions) are a
problem, the numbers start to lose meaning for people when they
are not easily recognizable. Yes, it can help when you're analyzing
a log file, but it seems to me that you would want to track the
host state throughout anyway, so you could just follow the number
as it changes.

If we change the behavior, we have to change documentation, and
our support people will get calls. But that's not a reason not
to do it.

-Ewan

2015-12-13 19:17:17

by Lee Duncan

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On 12/11/2015 07:31 AM, Ewan Milne wrote:
> On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
>> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>>>> "Lee" == Lee Duncan <[email protected]> writes:
>>>
>>> Lee> Martin: I will be glad to update the patch, creating a modprobe
>>> Lee> parameter as suggested, if you find this acceptable.
>>>
>>> For development use a module parameter would be fine. But I am concerned
>>> about our support folks that rely on the incrementing host number when
>>> analyzing customer log files.
>>>
>>> Ewan: How do you folks feel about this change?
>>>
>>
>> Ewan?
>
>
> Personally, I think having host numbers that increase essentially
> without limit (I think I've seen this with iSCSI sessions) are a
> problem, the numbers start to lose meaning for people when they
> are not easily recognizable. Yes, it can help when you're analyzing
> a log file, but it seems to me that you would want to track the
> host state throughout anyway, so you could just follow the number
> as it changes.
>
> If we change the behavior, we have to change documentation, and
> our support people will get calls. But that's not a reason not
> to do it.
>
> -Ewan
>

Ewan:

Thank you for your reply. I agree with you, which is why I generated
this patch.

If we *do* make this change, do you think it would be useful to have a
module option to revert to the old numbering behavior? I actually think
it would be more confusing to support two behaviors than it would be to
bite the bullet (so to speak) and make the change.

--
Lee Duncan

2015-12-14 15:07:16

by Ewan Milne

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On Sun, 2015-12-13 at 11:16 -0800, Lee Duncan wrote:
> On 12/11/2015 07:31 AM, Ewan Milne wrote:
> > On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
> >> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
> >>>>>>>> "Lee" == Lee Duncan <[email protected]> writes:
> >>>
> >>> Lee> Martin: I will be glad to update the patch, creating a modprobe
> >>> Lee> parameter as suggested, if you find this acceptable.
> >>>
> >>> For development use a module parameter would be fine. But I am concerned
> >>> about our support folks that rely on the incrementing host number when
> >>> analyzing customer log files.
> >>>
> >>> Ewan: How do you folks feel about this change?
> >>>
> >>
> >> Ewan?
> >
> >
> > Personally, I think having host numbers that increase essentially
> > without limit (I think I've seen this with iSCSI sessions) are a
> > problem, the numbers start to lose meaning for people when they
> > are not easily recognizable. Yes, it can help when you're analyzing
> > a log file, but it seems to me that you would want to track the
> > host state throughout anyway, so you could just follow the number
> > as it changes.
> >
> > If we change the behavior, we have to change documentation, and
> > our support people will get calls. But that's not a reason not
> > to do it.
> >
> > -Ewan
> >
>
> Ewan:
>
> Thank you for your reply. I agree with you, which is why I generated
> this patch.
>
> If we *do* make this change, do you think it would be useful to have a
> module option to revert to the old numbering behavior? I actually think
> it would be more confusing to support two behaviors than it would be to
> bite the bullet (so to speak) and make the change.
>

I'm not opposed to having the module option if others (Martin?) feel
they need it, but generally I think it's better to keep things as simple
as possible. So, unless there are strong objections, I would say no.

-Ewan


2015-12-14 15:29:30

by Hannes Reinecke

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On 12/14/2015 04:07 PM, Ewan Milne wrote:
> On Sun, 2015-12-13 at 11:16 -0800, Lee Duncan wrote:
>> On 12/11/2015 07:31 AM, Ewan Milne wrote:
>>> On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
>>>> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>>>>>> "Lee" == Lee Duncan <[email protected]> writes:
>>>>>
>>>>> Lee> Martin: I will be glad to update the patch, creating a modprobe
>>>>> Lee> parameter as suggested, if you find this acceptable.
>>>>>
>>>>> For development use a module parameter would be fine. But I am concerned
>>>>> about our support folks that rely on the incrementing host number when
>>>>> analyzing customer log files.
>>>>>
>>>>> Ewan: How do you folks feel about this change?
>>>>>
>>>>
>>>> Ewan?
>>>
>>>
>>> Personally, I think having host numbers that increase essentially
>>> without limit (I think I've seen this with iSCSI sessions) are a
>>> problem, the numbers start to lose meaning for people when they
>>> are not easily recognizable. Yes, it can help when you're analyzing
>>> a log file, but it seems to me that you would want to track the
>>> host state throughout anyway, so you could just follow the number
>>> as it changes.
>>>
>>> If we change the behavior, we have to change documentation, and
>>> our support people will get calls. But that's not a reason not
>>> to do it.
>>>
>>> -Ewan
>>>
>>
>> Ewan:
>>
>> Thank you for your reply. I agree with you, which is why I generated
>> this patch.
>>
>> If we *do* make this change, do you think it would be useful to have a
>> module option to revert to the old numbering behavior? I actually think
>> it would be more confusing to support two behaviors than it would be to
>> bite the bullet (so to speak) and make the change.
>>
>
> I'm not opposed to having the module option if others (Martin?) feel
> they need it, but generally I think it's better to keep things as simple
> as possible. So, unless there are strong objections, I would say no.
>
Agreeing with Ewan here.

Martin, I guess it's up to you to tell us whether you absolutely
need a module parameter ...

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

2015-12-15 01:55:47

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

>>>>> "Hannes" == Hannes Reinecke <[email protected]> writes:

>> I'm not opposed to having the module option if others (Martin?) feel
>> they need it, but generally I think it's better to keep things as
>> simple as possible. So, unless there are strong objections, I would
>> say no.

Hannes> Agreeing with Ewan here.

Hannes> I guess it's up to you to tell us whether you absolutely need a
Hannes> module parameter ...

Still not a big ida fan but since the most people seem to be in favor of
this I guess I'll have to bite the bullet.

I don't see much value in the module parameter since it will require
customers to tweak their configs and reproduce. Not worth the hassle.

--
Martin K. Petersen Oracle Linux Engineering

2015-12-17 19:25:44

by Lee Duncan

[permalink] [raw]
Subject: Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

On 12/14/2015 05:55 PM, Martin K. Petersen wrote:
>>>>>> "Hannes" == Hannes Reinecke <[email protected]> writes:
>
>>> I'm not opposed to having the module option if others (Martin?) feel
>>> they need it, but generally I think it's better to keep things as
>>> simple as possible. So, unless there are strong objections, I would
>>> say no.
>
> Hannes> Agreeing with Ewan here.
>
> Hannes> I guess it's up to you to tell us whether you absolutely need a
> Hannes> module parameter ...
>
> Still not a big ida fan but since the most people seem to be in favor of
> this I guess I'll have to bite the bullet.
>
> I don't see much value in the module parameter since it will require
> customers to tweak their configs and reproduce. Not worth the hassle.
>

Thank you Martin. I'll look at further cleaning up the host module, but
I think this still much better than leaving the code as is.

--
Lee Duncan