open-iscsi / rtslib-fb

Python library for configuring the Linux kernel-based multiprotocol SCSI target (LIO)
Apache License 2.0
73 stars 90 forks source link

Allow creating more than 256 LUNs per target #121

Closed pzakha closed 6 years ago

pzakha commented 6 years ago

Currently there's an unnecessary limitation of 256 LUNs per target, which is not enforced in the kernel driver. We keep a limit of 256 Mapped LUNs per initiator as required by the iSCSI protocol.

The end goal of this change is to be able to server many initiators using a single target. The value of 65535 was chosen arbitrarily as I'm not aware of the exact limit.

Note that targetcli also needs to be modified (slightly) to account for this change.

agrover commented 6 years ago

Hi, thanks for the PR!

So I flirted with a higher max LUN value a few years ago (1d99b471d5) but had to roll it back. I'm definitely in favor, but we need to check to make absolutely sure it's really supported. Narrowing it down to which kernel first had support would be even better.

pzakha commented 6 years ago

Hi Andy, thanks for the quick feedback! I'll do some more investigations, but from what I understand I shouldn't have any issues running this on latest Ubuntu for instance.

mikechristie commented 6 years ago

It was around this time:

commit 196e2e2aa362850bf45bcb14b9517124b23b921e Author: Hannes Reinecke hare@suse.de Date: Wed Jun 10 08:41:23 2015 +0200

target: Remove TARGET_MAX_LUNS_PER_TRANSPORT

I think this was the final commit in Hannes's set that converted from the old hard coded limit to just being limited by the 64bit lun definition.

agrover commented 6 years ago

@mikechristie thanks for the info!

I'm kinda thinking this might be a case where a version check might be good, to only allow >256 on kernel 4.2 or later. @pzakha are you up to adding that, or would you prefer we just merge this as is, and I can tackle that in a separate PR?

mikechristie commented 6 years ago

What happens in 4.1 if you try to make 257 LUNs? Does it not return a nice error when we do the link or mkdir step (I can't remember what command actually makes the LUN)?

agrover commented 6 years ago

lemme go check.

agrover commented 6 years ago

it does:

    if (mapped_lun > (TRANSPORT_MAX_LUNS_PER_TPG-1)) {
        pr_err("Mapped LUN: %lu exceeds TRANSPORT_MAX_LUNS_PER_TPG"
            "-1: %u for Target Portal Group: %u\n", mapped_lun,
            TRANSPORT_MAX_LUNS_PER_TPG-1,
            se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg));
        ret = -EINVAL;
        goto out;
    }

ok cool, so we don't need a version check.

I'm :+1: to merge this PR.

pzakha commented 6 years ago

Great. How should we handle synchronisation with targetcli? We need to change a reference from LUN.MAX_LUN to MappedLUN.MAX_LUN in class UILUNs.

agrover commented 6 years ago

@pzakha if you want to post a targetcli PR with the needed change then I will merge both.