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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel