Chris,
I'm sorry that none of us replied to your mail, but better 3 months late than
never, right?
Thanks very much for taking the time to go through the vmblock code and
cleaning it up. As a developer I can imagine how annoying it must be to
translate a non-trivial piece of code from one coding style to another, so I
very much appreciate your perseverance in the matter.
Before I get to your patch, let's discuss the more interesting issue of
vmblock's future. As you surmised, vmblock-fuse was written to replace vmblock.
This was done primarily because the informal feedback we received from some
kernel developers suggested that the design of vmblock is a non-starter for the
kernel. See http://bugzilla.redhat.com/show_bug.cgi?id=294341 for Christoph
Hellwig's comments and
http://sourceforge.net/mailarchive/message.php?msg_name=46E6FDEC.3090700%40codemonkey.ws
for Anthony Liguori's comments. Unfortunately, vmblock-fuse isn't quite ready
for prime-time, but until its code is part of open-vm-tools it's difficult to
describe exactly why (the open-sourcing process is taking longer than I
expected). I suggest that you table the work on vmblock for the time being, and
once vmblock-fuse is part of open-vm-tools we can have a more productive
discussion about the future of vmblock. How does that sound?
If you do want to continue preparing VMware module code for upstream inclusion,
first of all, thanks, and secondly, I recommend vmmemctl. It's a very simple
module (especially once the compat glue is removed), it hasn't changed in a
long time, and as Linux has recently gained kvm and xen balloon modules, I
imagine the VMware one will be fairly non-controversial. Another alternative is
the vmxnet module, which is a straightforward paravirtualized network card. The
one issue there might be the morphing "feature", which may require us to merge
the vmxnet code into the existing upstream pcnet32 module. If you're interested
in this, let me know and I'll provide more details.
Since we're on the subject, I also want to say a few words about upstreaming
VMware's kernel modules. In general we're getting more and more comfortable
about upstreaming as some of us have been socializing the idea within the
company. I think the remaining obstacle is this idea that once our code is
upstream, we are no longer acting as the distributor, which means we can no
longer control its destiny. We've shipped out-of-tree modules for years now and
have grown accustomed to the control that this gives us; among other things, we
can do things like release updated modules simultaneously with our products, or
make whatever changes we want regardless of how locked down the affected
distros might be (for example, we can add a new feature to a module intended
for a distro that's been declared end-of-life by its vendor). It's very
difficult to cede this control once you've enjoyed it for many product
releases, which is partly why progress has been slow. To mitigate this problem,
we're trying to understand whether, after upstreaming, we can gain some control
back by working more closely with distros so that new features or changes that
are important to us can be backported into existing distro releases in time
for, say, a VMware product release. I don't have any details to share yet, but
I'm sure either Ragavan or myself will say more about this in the near future.
Overall I think your vmblock patch is a solid improvement. I didn't look so
much at the functionality as I imagine it's the same as before, minus compat
glue. If there's something in particular you'd like me to pay closer attention
to, let me know. Since you're working on vmblock, I've attached two unit test
programs that you might find useful. vmblocktest.c is a general stress test
we've used on all of our vmblock implementations (fuse, Linux, FreeBSD, and
Solaris). manual-blocker.c was written by Daniel and used for testing
vmblock-fuse. You should be able to build both with a minimum of fuss, but let
me know if you have issues there.
Here are my (cosmetic) comments:
- The PURPOSE and TESTING portions are especially useful; we're suffering from
a dearth of documentation.
- The overloaded interface you mention in the test code was also discussed by
Dan Benamy and Miklos at the time that Dan was writing vmblock-fuse. In his
implementation he's opted for a new interface where a character representing
the command and the path are concatenated and sent to the driver via a single
write(2). You can see view the thread at
http://article.gmane.org/gmane.comp.file-systems.fuse.devel/6533, if you're
interested.
- In Kconfig, some of the capitalization of "VMware" is inconsistent; could you
fix that?
- Also in Kconfig, the driver should probably be referred to as a "blocking"
driver instead of a "block" driver; that should alleviate any confusion
regarding vmblock and the block layer.
- In Makefile, the indentation for the backticks isn't consistent. I'm not sure
if you care about that.
Again, thanks for helping us out with this effort!
/* **********************************************************
* Copyright (C) 2006 VMware, Inc. All Rights Reserved
* **********************************************************/
/*
* vmblocktest.c --
*
* Test program for the vmblock file system. Ensures correctness and
* stability.
*
*/
#if !defined(linux) && !defined(sun) && !defined(__FreeBSD__) &&
!defined(vmblock_fuse)
# error "vmblocktest.c needs to be ported to your OS."
#endif
#include <stdlib.h>
#include <stdio.h>
#include <pthread.h>
#include <libgen.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <errno.h>
#include <limits.h>
#include <signal.h>
#include <string.h>
#include <unistd.h>
#include <semaphore.h>
#ifdef sun
# include <stropts.h>
#endif
#include "vmblock.h"
#include "vm_basic_types.h"
#define BLOCKROOT VMBLOCK_MOUNT_POINT "/"
#define CONTROLFILE VMBLOCK_DEVICE
#define CONTROLFILE_MODE VMBLOCK_DEVICE_MODE
#define REALROOT "/tmp/VMwareDnD/"
#define FILENAME "/foo"
#define ACCESSORFULLNAME(dir) BLOCKROOT dir FILENAME
#define BLOCKERFULLNAME(dir) REALROOT dir
#define lprintf(...) \
do { \
pthread_mutex_lock(&print_lock); \
printf(__VA_ARGS__); \
fflush(stdout); \
pthread_mutex_unlock(&print_lock); \
} while(0)
#define lfprintf(stream, ...) \
do { \
pthread_mutex_lock(&print_lock); \
fprintf(stream, __VA_ARGS__); \
fflush(stream); \
pthread_mutex_unlock(&print_lock); \
} while(0)
#define Log(fmt, args...) lprintf(fmt, ## args)
#define ERROR(fmt, args...) lfprintf(stderr, fmt, ## args)
#define THREAD_LOG(fmt, args...) lprintf(" (%x) " fmt,
(unsigned)pthread_self(), ## args)
#define THREAD_ERROR(fmt, args...) lfprintf(stderr, " (%"FMTPID") " fmt,
getpid(), ## args)
#if defined (linux) || defined(__FreeBSD__)
# define os_thread_yield() sched_yield()
#elif defined(sun)
# define os_thread_yield() yield()
#endif
/*
* This program may optionally throttle accessor thread generation
* via POSIX semaphores.
*/
#if defined(USE_SEMAPHORES)
sem_t sem;
# define SEM_THREADS (unsigned int)10
# define SEM_INIT() sem_init(&sem, 0, SEM_THREADS)
# define SEM_DESTROY() sem_destroy(&sem)
# define SEM_WAIT() sem_wait(&sem)
# define SEM_POST() sem_post(&sem)
# define PTHREAD_SEMAPHORE_CLEANUP() pthread_cleanup_push(sem_post, \
(void *)&sem)
#else
# define SEM_INIT()
# define SEM_DESTROY()
# define SEM_WAIT()
# define SEM_POST()
# define PTHREAD_SEMAPHORE_CLEANUP()
#endif
/* Types */
typedef struct FileState {
/* Accessors will access the file through the vmblock namespace */
char *accessorName;
/* The blocker will add blocks using the real file's name */
char *blockerName;
Bool blocked;
unsigned int waiters;
} FileState;
typedef struct ThreadInfo {
int blockFd;
pthread_mutex_t *lock;
FileState *files;
size_t filesArraySize;
unsigned int sleepTime;
} ThreadInfo;
/* Variables */
static Bool programQuit = FALSE;
static FileState files[] = {
{ ACCESSORFULLNAME("0"), BLOCKERFULLNAME("0"), FALSE, 0 },
{ ACCESSORFULLNAME("1"), BLOCKERFULLNAME("1"), FALSE, 0 },
{ ACCESSORFULLNAME("2"), BLOCKERFULLNAME("2"), FALSE, 0 },
{ ACCESSORFULLNAME("3"), BLOCKERFULLNAME("3"), FALSE, 0 },
{ ACCESSORFULLNAME("4"), BLOCKERFULLNAME("4"), FALSE, 0 },
{ ACCESSORFULLNAME("5"), BLOCKERFULLNAME("5"), FALSE, 0 },
{ ACCESSORFULLNAME("6"), BLOCKERFULLNAME("6"), FALSE, 0 },
{ ACCESSORFULLNAME("7"), BLOCKERFULLNAME("7"), FALSE, 0 },
{ ACCESSORFULLNAME("8"), BLOCKERFULLNAME("8"), FALSE, 0 },
{ ACCESSORFULLNAME("9"), BLOCKERFULLNAME("9"), FALSE, 0 },
};
/* Thread entry points */
static void *blocker(void *arg);
static void *accessor(void *arg);
/* Utility functions */
static Bool addBlock(int fd, char const *filename);
static Bool delBlock(int fd, char const *filename);
static Bool listBlocks(int fd);
#ifdef VMBLOCK_PURGE_FILEBLOCKS
static Bool purgeBlocks(int fd);
#endif
static unsigned int getRand(unsigned int max);
static void sighandler(int sig);
pthread_mutex_t print_lock;
/*
*----------------------------------------------------------------------------
*
* main --
*
* Does all necessary setup then starts the blocker thread and continually
* starts accessor threads.
*
* Results:
* EXIT_SUCCESS and EXIT_FAILURE.
*
* Side effects:
* None.
*
*----------------------------------------------------------------------------
*/
int
main(int argc,
char *argv[])
{
int ret = EXIT_SUCCESS;
int i;
int blockFd;
char *progname;
pthread_t blockerThread;
pthread_mutex_t filesLock;
pthread_attr_t attr;
ThreadInfo info;
int count;
progname = basename(argv[0]);
pthread_mutex_init(&filesLock, NULL);
SEM_INIT();
pthread_mutex_init(&print_lock, NULL);
/* Open device user to add/delete blocks */
blockFd = open(CONTROLFILE, CONTROLFILE_MODE);
if (blockFd < 0) {
ERROR("%s: could not open " CONTROLFILE "\n", progname);
exit(EXIT_FAILURE);
}
/* Provide ability to just list blocks */
if (argc > 1) {
if (strcmp(argv[1], "-list") == 0) {
listBlocks(blockFd);
#ifdef VMBLOCK_PURGE_FILEBLOCKS
} else if (strcmp(argv[1], "-purge") == 0) {
purgeBlocks(blockFd);
#endif
}
close(blockFd);
exit(EXIT_SUCCESS);
}
/* Create directories/files used during test */
for (i = 0; i < sizeof files/sizeof files[0]; i++) {
int err;
struct stat statbuf;
char buf[PATH_MAX];
err = stat(files[i].blockerName, &statbuf);
if (!err) {
if (S_ISDIR(statbuf.st_mode)) {
goto create_file;
}
ERROR("%s: file [%s] already exists and is not a directory\n",
progname, files[i].blockerName);
goto exit_failure;
}
err = mkdir(files[i].blockerName, S_IRWXU | S_IRWXG);
if (err) {
ERROR("%s: could not create [%s]\n", progname, files[i].blockerName);
goto exit_failure;
}
create_file:
strncpy(buf, files[i].blockerName, sizeof buf);
strncat(buf, FILENAME, sizeof buf - strlen(files[i].blockerName));
err = stat(buf, &statbuf);
if (!err) {
if (S_ISREG(statbuf.st_mode)) {
continue;
}
ERROR("%s: file [%s] already exists and is not a regular file\n",
progname, buf);
goto exit_failure;
}
err = creat(buf, S_IRUSR | S_IRGRP);
if (err < 0) {
ERROR("%s: could not create [%s]\n", progname, buf);
goto exit_failure;
}
continue;
exit_failure:
close(blockFd);
exit(EXIT_FAILURE);
}
if (signal(SIGINT, sighandler) == SIG_ERR ||
signal(SIGTERM, sighandler) == SIG_ERR) {
ERROR("%s: could not install signal handlers\n", progname);
close(blockFd);
exit(EXIT_FAILURE);
}
/* Seems cleaner than a bunch of globals ... */
info.blockFd = blockFd;
info.lock = &filesLock;
info.files = files;
info.filesArraySize = sizeof files/sizeof files[0];
info.sleepTime = 1;
/*
* Start a thread that flips a random file's state, then sleeps for a while
* and does it again.
*/
if (pthread_create(&blockerThread, NULL, blocker, &info)) {
ERROR("%s: could not create blocker thread\n", progname);
close(blockFd);
exit(EXIT_FAILURE);
}
/*
* Start a bunch of threads that try to open a random file, check its status
* once they have it open (to make sure it is not blocked), then close it
* and exit.
*/
pthread_attr_init(&attr);
pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
count = 0;
while (!programQuit) {
pthread_t thread;
int rv;
SEM_WAIT();
/* Create threads until we cannot anymore */
if ((rv = pthread_create(&thread, &attr, accessor, &info)) != 0) {
SEM_POST();
ERROR("%s: could not create an accessor thread (%d total)\n",
progname, count);
ERROR("%s: pthread_create: %s\n", progname, strerror(rv));
if (rv == EAGAIN || rv == ENOMEM) {
os_thread_yield();
continue;
}
ret = EXIT_FAILURE;
break;
}
count++;
}
Log("%s: Not creating any more accessor threads.\n", progname);
programQuit = TRUE;
pthread_join(blockerThread, NULL);
pthread_mutex_destroy(&filesLock);
close(blockFd);
Log("%s: Exiting with %s.\n",
progname, ret == EXIT_SUCCESS ? "success" : "failure");
exit(ret);
}
/*
*----------------------------------------------------------------------------
*
* blocker --
*
* Entry point for the single blocker thread. Continuously picks a file at
* random and changes its state by adding or deleting a block on that file.
*
* Results:
* EXIT_SUCCESS and EXIT_FAILURE.
*
* Side effects:
* None.
*
*----------------------------------------------------------------------------
*/
static void *
blocker(void *arg) // IN
{
ThreadInfo *info = (ThreadInfo *)arg;
unsigned int index;
if (!info) {
THREAD_ERROR("blocker: no thread info provided\n");
pthread_exit((void *)EXIT_FAILURE);
}
while (!programQuit) {
index = getRand(info->filesArraySize - 1);
pthread_mutex_lock(info->lock);
if (info->files[index].blocked) {
info->files[index].blocked = FALSE;
if ( !delBlock(info->blockFd, info->files[index].blockerName) ) {
THREAD_ERROR("blocker: could not delete block on [%s]\n",
info->files[index].blockerName);
goto error;
}
} else if (info->files[index].waiters == 0) {
/*
* Only add a new block if all previous waiters are done. This
* ensures we don't get incorrect error messages from accessor threads
* even though the open(2) and check are not atomic in the accessor.
*/
info->files[index].blocked = TRUE;
if ( !addBlock(info->blockFd, info->files[index].blockerName) ) {
THREAD_ERROR("blocker: could not add block on [%s]\n",
info->files[index].blockerName);
goto error;
}
}
pthread_mutex_unlock(info->lock);
sleep(info->sleepTime);
}
pthread_mutex_lock(info->lock);
for (index = 0; index < info->filesArraySize; index++) {
if (info->files[index].blocked) {
info->files[index].blocked = FALSE;
#ifndef TEST_CLOSE_FD
THREAD_LOG("blocker: deleting block for [%s]\n",
info->files[index].blockerName);
if ( !delBlock(info->blockFd, info->files[index].blockerName) ) {
THREAD_ERROR("blocker: could not delete existing block on exit for
[%s]\n",
info->files[index].blockerName);
}
#else
THREAD_LOG("blocker: unmarking block for [%s], left for unblock on
release\n",
info->files[index].blockerName);
#endif
}
}
pthread_mutex_unlock(info->lock);
pthread_exit((void *)EXIT_SUCCESS);
error:
programQuit = TRUE;
pthread_mutex_unlock(info->lock);
pthread_exit((void *)EXIT_FAILURE);
}
/*
*----------------------------------------------------------------------------
*
* accessor --
*
* Entry point for accessor threads. Picks a file at random and attempts to
* open it. Once it is opened, ensures the state of the file is not blocked
* and outputs an error message accordingly.
*
* Results:
* EXIT_SUCCESS and EXIT_FAILURE.
*
* Side effects:
* None.
*
*----------------------------------------------------------------------------
*/
static void *
accessor(void *arg) // IN
{
ThreadInfo *info = (ThreadInfo *)arg;
unsigned int index;
int fd;
uintptr_t ret = EXIT_SUCCESS;
PTHREAD_SEMAPHORE_CLEANUP();
if (!info) {
THREAD_ERROR("blocker: no thread info provided\n");
pthread_exit((void *)EXIT_FAILURE);
}
index = getRand(info->filesArraySize - 1);
/*
* We can't hold the lock while calling open(2), since we will deadlock
* waiting for the blocker thread to remove the block on the file. So, we
* bump the waiter count to ensure that a new block is not placed on this
* file until we have checked its state. This prevents incorrect error
* messages that would happen if the blocker placed a new block on the file
* between our open(2) call returning and acquiring the lock.
*
* The fact that we can't hold the lock through open(2) also means that it's
* not atomic with respect to checking the file's blocked flag. Given this,
* it's possible that we'll miss some errors if the block is removed after
* open(2) returns but before we check the blocked flag -- hopefully running
* this test for a very long time will be sufficient to catch these cases.
* (Having the blocker sleep increases the likelihood of seeing such
* errors.)
*/
pthread_mutex_lock(info->lock);
info->files[index].waiters++;
pthread_mutex_unlock(info->lock);
fd = open(info->files[index].accessorName, O_RDONLY);
if (fd < 0) {
if (errno == EMFILE) {
/* We already have hit the maximum number of file descriptors */
pthread_mutex_lock(info->lock);
info->files[index].waiters--;
pthread_mutex_unlock(info->lock);
pthread_exit((void *)EXIT_FAILURE);
}
THREAD_ERROR("accessor: could not open file [%s]\n",
info->files[index].accessorName);
THREAD_ERROR("accessor: open: %s\n", strerror(errno));
pthread_exit((void *)EXIT_FAILURE);
}
pthread_mutex_lock(info->lock);
info->files[index].waiters--;
if (info->files[index].blocked) {
THREAD_ERROR("accessor: [ERROR] accessed file [%s] while blocked (%d)\n",
info->files[index].accessorName, info->files[index].blocked);
ret = EXIT_FAILURE;
}
pthread_mutex_unlock(info->lock);
close(fd);
pthread_exit((void *)ret);
}
/*
*----------------------------------------------------------------------------
*
* addBlock --
*
* Adds a block on the specified filename.
*
* Results:
* TRUE on success, FALSE on failure.
*
* Side effects:
* Future open(2)s on the file will block until delBlock() is called for
* that file.
*
*----------------------------------------------------------------------------
*/
static Bool
addBlock(int fd, // IN: fd of control device
char const *filename) // IN: filename to add block for
{
Log("Blocking [%s]\n", filename);
return VMBLOCK_CONTROL(fd, VMBLOCK_ADD_FILEBLOCK, filename) == 0;
}
/*
*----------------------------------------------------------------------------
*
* delBlock --
*
* Deletes a block on the specified filename.
*
* Results:
* TRUE on success, FALSE on failure.
*
* Side effects:
* Previous open(2)s that had blocked will now complete.
*
*----------------------------------------------------------------------------
*/
static Bool
delBlock(int fd, // IN: fd of control device
char const *filename) // IN: filename to delete block for
{
Log("Unblocking [%s]\n", filename);
return VMBLOCK_CONTROL(fd, VMBLOCK_DEL_FILEBLOCK, filename) == 0;
}
/*
*----------------------------------------------------------------------------
*
* listBlocks --
*
* Tells the kernel module to list all existing blocks.
*
* Results:
* TRUE on success, FALSE on failure.
*
* Side effects:
* None.
*
*----------------------------------------------------------------------------
*/
static Bool
listBlocks(int fd) // IN: fd of control device
{
Log("Listing blocks (check kernel log output)\n");
return VMBLOCK_CONTROL(fd, VMBLOCK_LIST_FILEBLOCKS, NULL) == 0;
}
#ifdef VMBLOCK_PURGE_FILEBLOCKS
/*
*----------------------------------------------------------------------------
*
* purgeBlocks --
*
* Tells the kernel module to purge all existing blocks, regardless of
* who opened them.
*
* Results:
* TRUE on success, FALSE on failure.
*
* Side effects:
* None.
*
*----------------------------------------------------------------------------
*/
static Bool
purgeBlocks(int fd) // IN: fd of control device
{
Log("Purging all blocks\n");
return VMBLOCK_CONTROL(fd, VMBLOCK_PURGE_FILEBLOCKS, NULL) == 0;
}
#endif
/*
*----------------------------------------------------------------------------
*
* getRand --
*
* Retrieves the next random number within the range [0, max].
*
* Results:
* A random number.
*
* Side effects:
* None.
*
*----------------------------------------------------------------------------
*/
static unsigned int
getRand(unsigned int max) // IN: max value returnable
{
return random() % max;
}
/*
*----------------------------------------------------------------------------
*
* sighandler --
*
* Sets the programQuit flag when a signal is received.
*
* Results:
* None.
*
* Side effects:
* Program will quit.
*
*----------------------------------------------------------------------------
*/
static void
sighandler(int sig)
{
programQuit = TRUE;
}
/* **********************************************************
* Copyright 2008 VMware, Inc. All rights reserved. -- VMware Confidential
* **********************************************************/
/*
* manual-blocker.c --
*
* A small test program for manually manipulating vmblock.
*/
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <limits.h>
#include <fcntl.h>
#include <string.h>
#include "vmblock.h"
/*
*-----------------------------------------------------------------------------
*
* main --
*
* Takes the target file to block as a command line arg. Sits in a loop
* waiting for commands.
*
* Results:
* Returns 0 on success and nonzero on failure.
*
* Side effects:
* None/all.
*
*-----------------------------------------------------------------------------
*/
int
main(int argc,
char *argv[])
{
int status;
char op;
if (argc < 2 ||
strcmp(argv[1], "-h") == 0 ||
strcmp(argv[1], "--help") == 0) {
printf("Usage: %s <path>\n", argv[0]);
puts("a to Add a block, d to Delete a block, or l to List blocks (to"
" vmblock's log).\n");
return 1;
}
int fd = open(VMBLOCK_DEVICE, VMBLOCK_DEVICE_MODE);
if (fd <= 0) {
perror("open");
return 2;
}
printf("Opened " VMBLOCK_DEVICE " as fd %d.\n", fd);
while (1) {
op = getchar();
if (op == 'a') {
status = VMBLOCK_CONTROL(fd, VMBLOCK_ADD_FILEBLOCK, argv[1]);
if (status != 0) {
perror(NULL);
} else {
printf("%s blocked.\n", argv[1]);
}
} else if (op == 'd') {
status = VMBLOCK_CONTROL(fd, VMBLOCK_DEL_FILEBLOCK, argv[1]);
if (status != 0) {
perror(NULL);
} else {
printf("%s unblocked.\n", argv[1]);
}
} else if (op == 'l') {
status = VMBLOCK_CONTROL(fd, VMBLOCK_LIST_FILEBLOCKS, argv[1]);
if (status != 0) {
perror(NULL);
} else {
printf("Check vmblock's log for list of blocks.\n");
}
}
}
return 0;
}
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
open-vm-tools-devel mailing list
open-vm-tools-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open-vm-tools-devel
