2:30 a.m.
I could feel that you want to express "Oh, shit! How terrible the code it
is." from the reply, although across the screen. Anyway, I'm really glad
to thanks for your review and suggestions. :-)
On 2018年02月08日 07:55, John Ferlan wrote:
That's where I'd expect to find the details of why someone
would want to
choose dlm over lockd or sanlock. What goodies are provided? What
advantage is there?
For those, assuming this scene: one customer has used cluster software
with DLM in their environment, why not choose DLM plugin instead of
sanlock/lockd ? Beside that, compared with sanlock/lockd, DLM don't need
the share storage, and sanlock would bring a little heavyheavily on disk
IO according to lockd RFC(2011-July/msg00337.html), but DLM only requires
network.
I have sent a RFC in 12/2017 to ack how the opinion is. Daniel said that
"it can be considered in scope for a libvirt lock manager plugin if you
want to try writing one"(2017-December/msg00706.html). So I draft this
patch set which could only make DLM run in libvirt(with configure flag
`--with-dlm` to compile 'dlm.so' module by force). Hope some suggestion
bringed from libvirt community to help me improve, if accepted DLM lock
manager plugin RFC, so I don't update 'docs/locking.html.in'.
-------------
The following is the reply contents, aim to answer why I do that
for most comments:
1) sorry for indent in my code. I coded them in two notebooks,
both using vim. However one is newer, I have not configured '.vimrc' in
that one.
2)
> > +AC_DEFUN([LIBVIRT_CHECK_CPG],[
> > + dnl in some distribution, Version is `UNKNOW` in libcpg.pc
UNKNOWN
doesn't make it very useful does it?
In some distribution, for example, openSUSE Tumbleweed:
$ cat /usr/lib64/pkgconfig/libcpg.pc
prefix=/usr
exec_prefix=${prefix}
libdir=/usr/lib64
includedir=${prefix}/include
Name: cpg
Version: UNKNOWN
Description: cpg
Requires:
Libs: -L${libdir} -lcpg
Cflags: -I${includedir}
using `LIBVIRT_CHECK_PKG([CPG], [libcpg], [xxx])` directly would make
build error. So I decide to ignore 'version'. I also don't know why
that is.
3)
> > +#define LOCK_RECORD_FILE_PATH
"/tmp/libvirtd-dlm-file"
Is somewhere in /tmp the best/only place to put this? What not similar
to lockd/sanlock using /var/lib/libvirt/dlm... (which would seemingly
need to be created on installation).
In my opinion, if one node reboots, DLM cluster would drop locks belong
that node, most of Linux distribution would clean '/tmp' directory...
4)
> > +
> > +#define STATUS "STATUS"
> > +#define RESOURCE_NAME "RESOURCE_NAME"
> > +#define LOCK_ID "LOCK_ID"
> > +#define LOCK_MODE "LOCK_MODE"
> > +#define VM_PID "VM_PID"
> > +
> > +#define BUFFERLEN 128
Yuck - for a stack variable...
> > + for (i = 0, str = raw, status = 0; ; str = NULL, i++)
{
> > + subtoken = strtok_r(str, " \n", &saveptr);
> > + if (subtoken == NULL)
> > + break;
> > +
> > + switch(i) {
> > + case 0:
These are magic numbers?
Badly code wants to provided a readable file, like that:
STATUS RESOURCE_NAME LOCK_MODE VM_PID
0
1501e418dedd122b050c91ec61650ae30d3c217806125d3c2f365989143aa28d
EXMODE 21857
The first line is the form header, the following is the lock
information.
5)
> > + char *name;
> > + uint32_t mode;
> > + uint32_t lkid;
I would think uint32_t would be limiting... Why not just unsigned int?
view in '/usr/include/libdlm.h', it use `uint32_t` instead of
`unsigned int`.
6)
> > + virMutexLock(&(lockListWait.listMutex));
> > + virListAddTail(&lock->entry, &(lockListWait.list));
> > + virMutexUnlock(&(lockListWait.listMutex));
What is this list for? I think this also is completely separable. I
wouldn't use a list either... 10s of domains probably works fine. 100s
of domains perhaps not as well 1000s of domains starts being a
bottleneck for sure.
Hashtable is a good idea, and I wanted to use it but I had never read
it's code... Chosing list because that there won't be too much vms in
one host, is it offten common that there is more than 50 vms in normal
usage?
7)
> > + if (safewrite(fd, buffer, strlen(buffer)) !=
strlen(buffer)) {
> > + virReportSystemError(errno,
> > + _("unable to write lock information
'%s' to file '%s'"),
> > + buffer,
NULLSTR(driver->lockRecordFilePath));
> > + return;
> > + }
Not sure I understand the reason of the file
Compared with sanlock and lockd, this implement of DLM has no daemon,
some area is needed to record lock information in order to resume lock
after libvirtd reboot. I chose file.
8)
> > + * found with another mode, and -ENOENT if no
orphan.
> > + *
> > + * cast/bast/param are (void *)1 because the kernel
> > + * returns errors if some are null.
> > + */
> > +
> > + status = dlm_ls_lockx(lockspace, mode, &lksb,
LKF_PERSISTENT|LKF_ORPHAN,
> > + name, strlen(name), 0,
> > + (void *)1, (void *)1, (void *)1,
^^ These would seem to relate to the same parametes used for
dlm_lock[_wait]. They are actually quite useful when it comes to lock
conversions. You may want to considering investigating that...
Still seems like a "bug" to me to need to pass 1 as a callback function
or a blocking callback function.
> > + NULL, NULL);
Why not dlm_is_lock() instead since you're not using xid and timeout
The code is from lvm2, I try to use `dlm_lock_wait`, however it failed,
I don't know why. `(void *)1` is the same reason. Maybe a bug from
libdlm or 'dlm.ko'.
9)
> > +
> > + /* create thread to receive notification from kernel */
notification for what? what gets called?
This is implemented by libdlm, the result is writted in `lksb`.
int dlm_lock_wait(uint32_t mode,
struct dlm_lksb *lksb,
uint32_t flags,
const void *name,
unsigned int namelen,
uint32_t parent, /* unused */
void *bastarg,
void (*bastaddr) (void *bastarg),
void *range); /* unused */
10)
> > + priv->hasRWDisks = true;
> > + /* ignore disk resource without error */
> > + return 0;
> > + }
> > + }
> > +
> > + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name,
&newName) < 0)
> Interesting _lockd uses SHA256 and _sanlock uses MD5...
> > + /* we need format the lock information, so the lock name
must be
the constant length */
> > + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256,
name,
&newName) < 0)
> > + goto cleanup;
Neither _lockd nor _sandisk do any sort of Hash on this name.
> > + memset(&lksb, 0, sizeof(lksb));
> > + rv = dlm_ls_lock_wait(lockspace, priv->resources[i].mode,
> > + &lksb, LKF_NOQUEUE|LKF_PERSISTENT,
> > + priv->resources[i].name, strlen(priv->resources[i].name),
This is where I wonder why we cannot pass the path to the disk to dlm
instead of the hash
It's interesting here. Originally I use MD5 instead of SHA256, but I
found that:
typedef enum {
VIR_CRYPTO_HASH_MD5, /* Don't use this except for historic
compat */
VIR_CRYPTO_HASH_SHA256,
VIR_CRYPTO_HASH_LAST
} virCryptoHash;
Futhermore, I find the bug patch related sanlock name: sanlock's name
length is limited 48, there is always someone use more than 48 characters
(see https://bugzilla.redhat.com/show_bug.cgi?id=735443). The same
situation, why not use the constant length? DLM limits name is 64 bytes
to avoid some potential bugs. Besides, it's convenient to format write
it to file.
11)
> > + * 'ensure sanlock socket is labelled with the VM
process label',
> > + * however, fixing sanlock socket security labelling remove
related
> > + * code. Now, `fd` parameter is useless.
> > + */
> > + if (fd)
> > + *fd = -1;
But you just set it to -1??!
Maybe I can submit a patch to cut those code. Afterall it's useless now.
12)
> > + virCheckFlags(0, -1);
> > +
> > + if (state)
> > + *state = NULL;
Why would this return NULL? My recollection is there is some sort of
inquiry API for dlm.
Inquiry API is disappeared now... Only lock/unlock refered to libdlm
source code and reference book.
http://people.redhat.com/ccaulfie/docs/rhdlmbook.pdf
Chapter 4.3. Lock Query Operations
--
Regards
Lin Fu(river)