gmane.linux.scsi.target.devel

Re: Problems with multipathing between VMWare ESXi and LIO

Nicholas A. Bellinger — 2013-05-24T20:03:19

Hi Eljas,

On Wed, 2013-05-22 at 12:17 +0000, Eljas Alakulppi wrote:

Btw, if your going to run multiple network portals on the same subnet,
you'll need separate VLANs to isolate traffic amongst the ports,
otherwise you'll end up seeing strange scenarios where outgoing traffic
is getting sent on the wrong Ethernet port. 


Mmm, this would seem to indicate that something is afoul below the
application layer..


Can you verify if your able to reproduce without jumbo frames enabled..?


This looks sane..

--nab

Re: [GIT PULL] target fixes for v3.10-rc2

Or Gerlitz — 2013-05-22T20:38:32

On Thu, May 16, 2013 at 7:53 AM, Nicholas A. Bellinger
<nab< at >linux-iscsi.org> wrote:


Cool!


are you merging this into the lio-utils git tree and maybe sparing a
word on that in the documentation (where?) we'd like to point people
to these bits and guidelines.

Or.

Problems with multipathing between VMWare ESXi and LIO

Eljas Alakulppi — 2013-05-22T12:17:41

Hello.

I'm trying to get multipathing working between LIO target and VMWare ESXi 5.1 U1. However, I'm not having a lot of luck as I'm not entirely sure which side is the one not playing nice. From VMWare side, I can see that it's using each interface, but it's only managing to connect to single interface on the target. 

############
Runtime nameTargetLUN
vmhba37:C4:T0:L0iqn.2013-02.fi.splab.storage.iscsi.tibbers:vmware:10.255.10.5:32600
vmhba37:C4:T0:L1iqn.2013-02.fi.splab.storage.iscsi.tibbers:vmware:10.255.10.5:32601
vmhba37:C4:T0:L2iqn.2013-02.fi.splab.storage.iscsi.tibbers:vmware:10.255.10.5:32602
vmhba37:C5:T0:L0iqn.2013-02.fi.splab.storage.iscsi.tibbers:vmware:10.255.10.5:32600
vmhba37:C5:T0:L1iqn.2013-02.fi.splab.storage.iscsi.tibbers:vmware:10.255.10.5:32601
vmhba37:C5:T0:L2iqn.2013-02.fi.splab.storage.iscsi.tibbers:vmware:10.255.10.5:32602
vmhba37:C6:T0:L0iqn.2013-02.fi.splab.storage.iscsi.tibbers:vmware:10.255.10.5:32600
vmhba37:C6:T0:L1iqn.2013-02.fi.splab.storage.iscsi

Re: [PATCH 1/3] target/rd: Add ramdisk bit for NULLIO operation

Andy Grover — 2013-05-20T23:50:11

Nice. Can we go farther, set nullio on virtual_lun0, and not allocate 
backing pages for nullio ram devices? Or would that break something?

Re: FC targetcli difficulties

Nicholas A. Bellinger — 2013-05-20T20:06:27

Hi Craig,

Btw, if your using IBLOCK w/ WriteCacheEnable=1 + FUADPO=1 settings for
tyour MD RAID backend device, you'll need the v3.9.3 kernel, or apply
the following regression bugfix patch:

http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-3.9.y&id=1ed59c216723ca3586509ff309fbb987312d922a

Otherwise, you'll not be able to WRITE to IBLOCK w/ WCE=1 + FUADPO=1
settings.

--nab

Re: writing config to filesystem (was: Re: [PATCHv2 0/12] target_core_pr.c cleanups)

Nicholas A. Bellinger — 2013-05-20T20:00:18

Again, look closer at the code.  ALL_TG_PT is one example where someone
randomly reading aptpl metadata from configfs can get the incorrect
state, but there are certainly more cases than that.

There's a reason why aptpl updates are currently performed at specific
times in the PR logic path, and it's an incorrect assumption that they
can be read at any time from user-space and get the correct state for
all registrations and reservations.


The only policy is the the hardcoded PR aptpl metadata path, and like I
said before, go ahead and submit a patch to allow the changing of the
path.

Aside from that, and given PR aptpl metadata is not being *read* by
kernel-space, your argument for an upcall and user-space dependency in
an existing I/O path is extremely thin.


The more I think about this, the less and less sense it makes any sense
to me.

Why you think that exporting possibly dozens of key=value formatted PR
registrations via a single configfs attribute is OK is beyond me, but it
certainly breaks every r

writing config to filesystem (was: Re: [PATCHv2 0/12] target_core_pr.c cleanups)

Andy Grover — 2013-05-20T18:39:00

Why would reading aptpl metadata from configfs lead to incorrect state?


My example was crappy. vfs_write has its uses -- I have no problem with 
fileio backstore using it -- but basically the kernel should not (and 
generally does not) write configuration info to the filesystem; config 
is pushed down into the kernel by userspace, and read by userspace via 
mechanisms besides the kernel writing files.


Here's an excerpt from an article GregKH (CCd) wrote:

(talks about reading config from fs but also applies to writing)

http://www.linuxjournal.com/article/8110

"Trying to protect the kernel from dumb programming errors is not the 
most important reason for not allowing drivers to read files. The 
biggest issue is policy. Linux kernel programmers try to flee from the 
word policy as fast as they can. They almost never want to force the 
kernel to force a policy on to user space that can possibly be avoided. 
Having a module read a file from a filesystem at a specific location 
forces the policy of the lo

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Andy Grover — 2013-05-20T16:53:21

Ah, never mind.. if we're defining a new configfs interface and went 
with per-reservation entries, this isn't an issue.

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Andy Grover — 2013-05-20T16:43:13


Ok, how big must we handle? PR_APTPL_BUF_LEN is currently 8192.

Regards -- Andy

Re: FC targetcli difficulties

Andy Grover — 2013-05-20T16:38:59


Hi Craig,

I think you're running into

https://github.com/agrover/rtslib-fb/issues/23

Please update to latest targetcli and rtslib:
targetcli-2.1.fb25-1.fc18
python-rtslib-2.1.fb34-1.fc18

via 'yum update targetcli --enablerepo=updates-testing'.

(This should also be in the regular 'updates' repo by tomorrow.)

Fedora ships a modified version of these tools, which accounts for the 
other differences you noted. Please feel free to use the targetcli-fb 
list (CCd) if you encounter further issues.

Regards -- Andy

FC targetcli difficulties

Craig Watson — 2013-05-19T17:10:58

Hi,

I am attempting to set up SAN access to a RAID on a Linux platform using 
LIO/TMC with a QLogic qle2562 Fiber Channel HBA.  I have attempted to 
follow the example shown at 
http://www.linux-iscsi.org/wiki/Fibre_Channel#Enable_target_mode and 
have run into some problems.  Details of my issues are after the 
debug/system info below suggested at 
http://linux-iscsi.org/wiki/Support. Sorry it is so long but that's what 
was asked for.


System:
    ASUS DSEB-DG motherboard w/2 3 GHZ Quad-Core Xeon processors, 4 GB RAM.
    Seagate 750 GB SATA system drive
    NVIDIA Quadro FX 570 video card
    8 disk Linux Software RAID
    QLogic qle2562 Fiber Channel HBA

    Newly installed Fedora Core 18 updated to latest 3.9 kernel.

     root< at >buickgn ~ # cat /etc/issue
     Fedora release 18 (Spherical Cow)
     Kernel \r on an \m (\l)

     root< at >buickgn ~ # uname -a
     Linux buickgn.mydomain.com 3.9.2-200.fc18.x86_64 #1 SMP Mon May 13 
13:59:47 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

===========================

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Nicholas A. Bellinger — 2013-05-18T01:46:10

<SNIP>


Also, I don't want aptpl metadata to be limited by PAGE_SIZE output in
configfs, so unless your planning to fix that for the long term as well
don't bother sending me patches for user-space upcalls until that is
resolved first.

--nab

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Nicholas A. Bellinger — 2013-05-18T01:35:41

No, it's the pulling out of aptpl metadata from configfs at random times
that can lead to incorrect state.  Just having that bit exposed in patch
#12 doesn't make any sense given the NAK on the original #13, which is
why I'm not applying it either.


I'm singling you out because of things in the recent past that I had to
fix due to general laziness, such as in commit f80e8ed3.

Really, don't ever send large changes to key parts of I/O path code
with bugs that would have be immediately spotted if you'd bother to run
a single block of I/O before sending the changes my way.


Huh..?  kernel_write translates to vfs_write..  So your saying that
these are going away long-term..?


Nonsense, this has nothing to do with policy.

Whatever you want to do can be done with the existing code using inotify
on the metadata files in question, with the exact same formatting as the
changes you've already proposed.


Sorry, but I'll be NAKing anything that does an user-space upcall and
keeps two diverging paths for user-space

[PATCH 2/3] target: Re-instate sess_wait_list for target_wait_for_sess_cmds

Nicholas A. Bellinger — 2013-05-18T00:40:30

From: Nicholas Bellinger <nab< at >linux-iscsi.org>

Switch back to pre commit 1c7b13fe652 list splicing logic for active I/O
shutdown with tcm_qla2xxx + ib_srpt fabrics.

The original commit was done under the incorrect assumption that it's safe to
walk se_sess->sess_cmd_list unprotected in target_wait_for_sess_cmds() after
sess->sess_tearing_down = 1 has been set by target_sess_cmd_list_set_waiting()
during session shutdown.

So instead of adding sess->sess_cmd_lock protection around sess->sess_cmd_list
during target_wait_for_sess_cmds(), switch back to sess->sess_wait_list to
allow wait_for_completion() + TFO->release_cmd() to occur without having to
walk ->sess_cmd_list after the list_splice.

Also add a check to exit if target_sess_cmd_list_set_waiting() has already
been called, and add a WARN_ON to check for any fabric bug where new se_cmds
are added to sess->sess_cmd_list after sess->sess_tearing_down = 1 has already
been set.

Cc: Joern Engel <joern< at >logfs.org>
Cc: Roland Dreier <roland< at >kernel.org>
Signed-

[PATCH 3/3] ib_srpt: Call target_sess_cmd_list_set_waiting during shutdown_session

Nicholas A. Bellinger — 2013-05-18T00:40:31

From: Nicholas Bellinger <nab< at >linux-iscsi.org>

Given that srpt_release_channel_work() calls target_wait_for_sess_cmds()
to allow outstanding se_cmd_t->cmd_kref a change to complete, the call
to perform target_sess_cmd_list_set_waiting() needs to happen in
srpt_shutdown_session()

Also, this patch adds an explicit call to srpt_shutdown_session() within
srpt_drain_channel() so that target_sess_cmd_list_set_waiting() will be
called in the cases where TFO->shutdown_session() is not triggered
directly by TCM.

Cc: Joern Engel <joern< at >logfs.org>
Cc: Roland Dreier <roland< at >kernel.org>
Signed-off-by: Nicholas Bellinger <nab< at >linux-iscsi.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |   32 ++++++++++++++++++++++++--------
 drivers/infiniband/ulp/srpt/ib_srpt.h |    1 +
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index c318f7c..824b704 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/i

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Andy Grover — 2013-05-17T22:36:22

Well I see that it becomes very difficult to assure that aptpl metadata 
changes will be saved if the system crashes less than a second after the 
update. The current pr save method is subject to this as well, and seems 
pretty unavoidable without killing performance. Is this the issue you're 
referring to?


Working code is not good enough for the Linux kernel. It needs to be 
maintainable, readable working code.

I'm fine with you pushing back on the testing part. I'll include testing 
summaries in my future patchsets. I'm not fine with you singling me out 
for blame, because we ALL need to get better about this.


It's not just that /var/targets/pr is a bad location, it's that the 
kernel should not write to the fs *at all*.

Do a 'git grep " kernel_write"'. We are on a short, short list of spots 
that do this, and we need to find a way to stop, long term. I understand 
existing user tools expect this, which is why my patch maintained 
current behavior for now, then down the road we stick in a deprecatio

Re: [PATCH] target: Use FD_MAX_SECTORS/FD_BLOCKSIZE for blockdevs using fileio

Greg-KH — 2013-05-17T21:07:16

Wonderful, thanks for doing this so quickly.

greg k-h

[PATCH] target: Use FD_MAX_SECTORS/FD_BLOCKSIZE for blockdevs using fileio

Nicholas A. Bellinger — 2013-05-17T20:29:41

From: Andy Grover <agrover< at >redhat.com>

This is a <= v3.9 backport of upstream commit e3e84cda32170

------------------------------------------------------------------

We can still see the error reported in

https://patchwork.kernel.org/patch/2338981/

when using fileio backed by a block device.

I'm assuming this will get us past that error (from sbc_parse_cdb),
and also assuming it's OK to have our max_sectors be larger than
the block's queue max hw sectors?

Reported-by: Eric Harney <eharney< at >redhat.com>
Signed-off-by: Andy Grover <agrover< at >redhat.com>
Signed-off-by: Nicholas Bellinger <nab< at >linux-iscsi.org>
---
 drivers/target/target_core_file.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 17a6acb..ca4b219 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
< at >< at > -148,13 +148,8 < at >< at > static int fd_configure_device(struct se_device *dev)
  */
 inode = file->f_mappin

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Nicholas A. Bellinger — 2013-05-17T20:12:35

No, because it assumes that userspace can poll configfs at anytime for
the correct state of in-flight PR aptpl metadata, which is an incorrect
assumption if you look closely at the code.


That's a good start.


The problem is that I've gotten large cleanup patches like this in the
past that obviously have had zero testing at all.

One example of this is commit f80e8ed3, which if you'd bother to do even
basic testing on it would have been noticed immediately.

Really, If you insist on churning working code for flexibilities sake,
don't be surprised if I push back on the testing part.


The question is why is this needed in this case..?

Why is it important enough to start adding multiple code-paths for
saving point in time state of PR + ALUA metadata by adding userspace
code dependencies within an SCSI I/O path..?

If it's just matter of where the save state is located, there's lots of
easier ways than adding a userspace up-call to an existing SCSI PR path.

Any why on earth does user-space care about the f

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Andy Grover — 2013-05-17T17:34:33

Well PR and ALUA are the only two spots in LIO where config is not in 
configfs. Shouldn't we start to fix this? This patch exposes PR, and it 
only adds 3 lines of code, seems like a good start.


I created a loopback device and poked it with sg_persist -Z, setting and 
clearing PRs. The writes to /var/target/pr still worked when 
/sbin/target_notify was absent, and were properly omitted when present 
and the upcall returned success. The configfs change worked as well.


Just for the record I do *not* have the most regressions, and I view 
falsely singling me out for public criticism as spillover from dislike 
for the upcall patch.

Can we have a purely technical discussion about the upcall patch now, 
please? You said:


The real benefit is removing the policy of where and how kernel state 
gets saved by userspace from the kernel, similar to how udev leaves it 
up to userspace on exactly how to layout /dev. The upcall gives 
userspace the flexibility to store PR info however it likes, instead of 
mandatin

Re: [PATCHv2 0/12] target_core_pr.c cleanups

Nicholas A. Bellinger — 2013-05-17T00:18:07

So I'm mostly OK with these. 

The one exception is with patch #12, which given the NAK on the upcall
junk doesn't make sense to apply as is.

From now on I'm going to be alot more strict is taking changes like this
that are only compile tested, given the number of regressions in the
past that have come up from your changes.  Can you please share how
you've been testing these changes, specifically the aptpl bits..?

Also, I'll not be applying these before rebasing target-pending/for-next
to -rc2 next week.

--nab