[pve-devel] [PATCH container 0/2] Linux LIO/targetcli support

[Udo Rader]

On Wed, 2018-06-13 at 09:31 +0200, Dominik Csapak wrote:
> hi, thanks for the patches
> 
> i looked briefly over them (and will dive deeper into the code in
> the 
> following days) and played a little around
> 
> here an initial review of what i saw/found
> 
> nitpicks:
> 
> you used the wrong git repository in the subject (container vs
> storage), 
> not really important, but you might want to correct this if you plan
> to 
> send patches in the future (makes it less confusing)

yes, sorry, only noticed that once I had sent them to the list.

> both patches have the same commit subject and no commit message
> it would be very good if the patches would describe more detailed
> what 
> they do

if I see it right, the commit messages were "adding linux LIO support",
but next time I'll try to add more info there. The cover letter should
however give more detailled information about the patch.

> if they really need to have the same commit subject, it would
> probably 
> be better to combine them into one commit

ok

> during my initial tests, it worked (mostly) but i found some strange 
> behaviours:
> 
> when we execute a zfs request not in a worker (e.g. a content
> listing)
> and then create a lun in a worker, only that worker, and no future 
> worker knows of it (because when we fork, we copy all data currently
> in 
> variables)
> 
> this seems due to the $SETTINGS variable which get filled whenever we
> get info from the target

can you please elaborate what a "worker" is in PVE speech? 

> it seems this is a general problem also for the other zfs over iscsi 
> providers, has anyone who uses them (@mir?) the same problems?
> how do you prevent/handle this?
> 
> a possible solution i guess would be to prevent filling the
> $SETTINGS variable when not in a worker, any other idea?

One of the things I found too about SETTINGS is that the other
providers somehow seem to use it as a "cache", in order to reduce the
amount of calls between the PVE and the target.

This has the dangerous drawback, that if some external process modifies
the target (ie adds or removes another LUN), PVE will probably never
pick up that change, because it relies on the assumption that the
target is only controlled by the PVE. I have not fully investigated
this though.

Originally I always repopulated the SETTINGS variable before running
any "LUN command", but after looking at the other implementations, I
decided to do it as they do it.

So the "easy" fix for this would probably be to revert that and to
repopulate the SETTINGS hash for each call.

Regards

Udo

-- 
Udo Rader, GF/CEO
BestSolution.at EDV Systemhaus GmbH
Eduard-Bodem-Gasse 5-7, A-6020 Innsbruck
http://www.bestsolution.at/
Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.proxmox.com/pipermail/pve-devel/attachments/20180615/9901d8cd/attachment.sig>