vfs_commit.c feature patch: sync at eof (before file close)
James Peach
jpeach at samba.org
Wed Nov 7 05:48:20 GMT 2007
On 31/10/2007, at 3:35 PM, <david.losada at urv.cat>
<david.losada at urv.cat> wrote:
> Hello,
Hi David,
thanks for the patch.
> here I'm attaching a patch for vfs_commit.c, against 3.0.26a
> release. It provides the following changes:
>
> 1. check errors after fsync
> Errors can arise due to fsync (or fdatasync) system calls. The
> current version of the module doesn't check for them, the patch
> addresses this.
>
> The exception is syncing before closing the file, errors are not
> checked then. My interpretation is that if errors are ignored when
> syncing, those errors will raise again when closing the file.
>
> 2. new optional feature: syncing at end of file
> This feature tries to address some woes we were suffering in our
> setup, with Samba over NFS quotas, (and Windows clients)
>
> The aim is to sync the file before the client decides to close it,
> so quota errors are reported at the right time for Windows clients.
> But, should sync as little as possible, so less performance is lost.
> The module allows the user to select different strategies:
>
> - predict (default): when copying files from the Windows explorer
> shell, the client declares how big is the file that is going to be
> copied. This translates into an ftruncate VFS call. This way, the
> module can tell when the last byte of file has been written, and
> sync immediately after.
>
> - strict: this option works when the user has set "strict allocate =
> yes". In this strategy, the module syncs whenever a write operation
> has resulted in an increased file size.
So predict and strict aren't really very different. In fact, for
Explorer copies, strict mode will effectively become predict mode.
I simplified and combined the modes to always track EOF and I think
the resulting behaviour should be equivalent.
Could you please review the attached patch?
-------------- next part --------------
diff --git a/source/modules/vfs_commit.c b/source/modules/vfs_commit.c
index 39de7f7..41bfeb3 100644
--- a/source/modules/vfs_commit.c
+++ b/source/modules/vfs_commit.c
@@ -1,5 +1,6 @@
/*
- * Copyright (c) James Peach 2006
+ * Copyright (c) James Peach 2006,2007
+ * Copyright (c) David Losada Carballo 2007
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -29,23 +30,96 @@
* Tunables:
*
* commit: dthresh Amount of dirty data that can accumulate
- * before we commit (sync) it.
+ * before we commit (sync) it.
*
* commit: debug Debug level at which to emit messages.
*
+ * commit: on eof Commit after writing what could be the last bytes
+ * of the file. This option doesn't behave as expected
+ * when having write caches enabled. Default = no.
+ *
+ * commit: eof mode String to tune when the module triggers the commit
+ * for end of file. Possible values:
+ *
+ * = predict Some clients declare the size of the file before
+ * transferring it. With this option, we remember that
+ * declaration, and commit after writing in that file
+ * position. If the client doesn't declare the size
+ * of file, commiting on EOF is not triggered.
+ *
+ * = strict ONLY in shares with "strict allocate" enabled. This
+ * mode triggers a commit whenever a write increases
+ * the size of the file.
+ *
+ * = combined ONLY in shares with "strict allocate" enabled. This
+ * mode enhances the strict behaviour with prediction.
+ * This is, if the client has declared the file size,
+ * commit is only triggered when that size has been
+ * reached. Further write calls that may increase that
+ * size are also commited. When no size declaration
+ * available, it behaves like the "strict" mode.
+ *
+ * TODO: explain why these last two modes are useful for NFS
*/
#define MODULE "commit"
static int module_debug;
+enum eof_mode
+{
+ EOF_NONE = 0x0000,
+ EOF_PREDICT = 0x0001,
+ EOF_STRICT = 0x0002,
+ EOF_COMBINED = 0x0003
+};
+
struct commit_info
{
+ /* For chunk-based commits */
SMB_OFF_T dbytes; /* Dirty (uncommitted) bytes */
SMB_OFF_T dthresh; /* Dirty data threshold */
+ /* For commits on EOF */
+ enum eof_mode on_eof;
+ SMB_OFF_T eof; /* Expected file size */
};
-static void commit_all(
+static int commit_get_size_pos(
+ struct vfs_handle_struct * handle,
+ files_struct * fsp,
+ int fd,
+ SMB_OFF_T *o_size, /* output param */
+ SMB_OFF_T *o_pos) /* output param */
+{
+ struct commit_info *c;
+
+ /* Do commit modes require us to do the fstat()? */
+ if ((c = VFS_FETCH_FSP_EXTENSION(handle, fsp)) &&
+ (c->on_eof != EOF_NONE)) {
+ SMB_STRUCT_STAT st;
+ if (SMB_VFS_FSTAT(fsp, fd, &st) == -1) {
+ return -1;
+ }
+ *o_size = st.st_size;
+ *o_pos = fsp->fh->pos;
+ }
+ return 0;
+}
+
+static int commit_do(
+ struct commit_info * c,
+ int fd)
+{
+ int result;
+
+ result = fdatasync(fd);
+ if (result == 0) {
+ c->dbytes = 0; /* on success, no dirty bytes */
+ }
+ return result;
+}
+
+static int commit_all(
struct vfs_handle_struct * handle,
files_struct * fsp)
{
@@ -57,34 +131,49 @@ static void commit_all(
("%s: flushing %lu dirty bytes\n",
MODULE, (unsigned long)c->dbytes));
- fdatasync(fsp->fh->fd);
- c->dbytes = 0;
+ return commit_do(c, fsp->fh->fd);
}
}
+ return 0;
}
-static void commit(
+static int commit_chunk(
struct vfs_handle_struct * handle,
files_struct * fsp,
+ SMB_OFF_T offset,
ssize_t last_write)
{
struct commit_info *c;
- if ((c = VFS_FETCH_FSP_EXTENSION(handle, fsp))) {
+ if ((c = VFS_FETCH_FSP_EXTENSION(handle, fsp)) == NULL) {
+ return 0;
+ }
- if (last_write > 0) {
- c->dbytes += last_write;
- }
+ c->dbytes += last_write; /* dirty bytes always counted */
- if (c->dbytes > c->dthresh) {
- DEBUG(module_debug,
- ("%s: flushing %lu dirty bytes\n",
- MODULE, (unsigned long)c->dbytes));
+ if (c->dthresh && (c->dbytes > c->dthresh)) {
+ DEBUG(module_debug,
+ ("%s: flushing %lu dirty bytes\n",
+ MODULE, (unsigned long)c->dbytes));
- fdatasync(fsp->fh->fd);
- c->dbytes = 0;
- }
- }
+ return commit_do(c, fsp->fh->fd);
+ }
+
+ if (c->on_eof == EOF_NONE) {
+ return 0;
+ }
+
+ /* This write hit or went past our current idea of the file size. */
+ if (offset + last_write >= c->eof) {
+ if (commit_do(c, fsp->fh->fd) == -1) {
+ return -1;
+ }
+
+ /* Adjust EOF upwards. */
+ c->eof = offset + last_write;
+ }
+
+ return 0;
}
static int commit_connect(
@@ -104,25 +193,71 @@ static int commit_open(
mode_t mode)
{
SMB_OFF_T dthresh;
+ bool on_eof;
+ const char *eof_mode;
+ bool strict_alloc;
+ struct commit_info *c = NULL;
+ int fd;
/* Don't bother with read-only files. */
if ((flags & O_ACCMODE) == O_RDONLY) {
return SMB_VFS_NEXT_OPEN(handle, fname, fsp, flags, mode);
}
+ /* Read and check module configuration */
dthresh = conv_str_size(lp_parm_const_string(SNUM(handle->conn),
MODULE, "dthresh", NULL));
+ on_eof = lp_parm_bool(SNUM(handle->conn), MODULE, "on eof", False);
+ eof_mode = lp_parm_const_string(SNUM(handle->conn),
+ MODULE, "eof mode", "predict");
+ strict_alloc = lp_strict_allocate(SNUM(fsp->conn));
- if (dthresh > 0) {
- struct commit_info * c;
+ if (dthresh > 0 || on_eof) {
c = VFS_ADD_FSP_EXTENSION(handle, fsp, struct commit_info);
+ /* Process main tunables */
if (c) {
c->dthresh = dthresh;
c->dbytes = 0;
+ c->on_eof = EOF_NONE;
+ c->eof = 0;
}
}
+ /* Process eof_mode tunable */
+ if (c && on_eof) {
+
+ if (!StrCaseCmp(eof_mode, "predict")) {
+ c->on_eof = EOF_PREDICT;
+ } else if (!StrCaseCmp(eof_mode, "strict")) {
+ c->on_eof = EOF_STRICT;
+ } else if (!StrCaseCmp(eof_mode, "combined")) {
+ c->on_eof = EOF_COMBINED;
+ }
- return SMB_VFS_NEXT_OPEN(handle, fname, fsp, flags, mode);
+ if ((c->on_eof & EOF_STRICT) && !strict_alloc) {
+ /* TODO: log to debug the disabling of this func. */
+ DEBUG(module_debug,
+ ("disabling strict EOF mode because "
+ "strict alloc is disabled\n"));
+ c->on_eof = EOF_NONE;
+ }
+ }
+
+ fd = SMB_VFS_NEXT_OPEN(handle, fname, fsp, flags, mode);
+ if (fd == -1) {
+ VFS_REMOVE_FSP_EXTENSION(handle, fsp);
+ return fd;
+ }
+
+ /* Do commit modes require us to do the fstat()? */
+ if (c && (c->on_eof != EOF_NONE)) {
+ SMB_STRUCT_STAT st;
+ if (SMB_VFS_FSTAT(fsp, fd, &st) == -1) {
+ return -1;
+ }
+ c->eof = st.st_size;
+ }
+
+ return 0;
}
static ssize_t commit_write(
@@ -135,7 +270,13 @@ static ssize_t commit_write(
ssize_t ret;
ret = SMB_VFS_NEXT_WRITE(handle, fsp, fd, data, count);
- commit(handle, fsp, ret);
+
+ if (ret > 0) { /* on write success */
+ /* Do commit logic, checking errors that can arise */
+ if (commit_chunk(handle, fsp, fsp->fh->pos, ret) == -1) {
+ return -1;
+ }
+ }
return ret;
}
@@ -151,20 +292,47 @@ static ssize_t commit_pwrite(
ssize_t ret;
ret = SMB_VFS_NEXT_PWRITE(handle, fsp, fd, data, count, offset);
- commit(handle, fsp, ret);
+
+ if (ret > 0) { /* on write success */
+ /* Do commit logic, checking errors that can arise */
+ if (commit_chunk(handle, fsp, offset, ret) == -1) {
+ return -1;
+ }
+ }
return ret;
}
-static ssize_t commit_close(
+static int commit_close(
vfs_handle_struct * handle,
files_struct * fsp,
int fd)
{
+ /* Commit errors not checked, close() will find them again */
commit_all(handle, fsp);
return SMB_VFS_NEXT_CLOSE(handle, fsp, fd);
}
+static int commit_ftruncate(
+ vfs_handle_struct * handle,
+ files_struct * fsp,
+ int fd,
+ SMB_OFF_T len)
+{
+ int result;
+
+ result = SMB_VFS_NEXT_FTRUNCATE(handle, fsp, fd, len);
+ if (result == 0) {
+ struct commit_info *c;
+ if ((c = VFS_FETCH_FSP_EXTENSION(handle, fsp))) {
+ commit_chunk(handle, fsp, len, 0);
+ c->eof = len;
+ }
+ }
+
+ return result;
+}
+
static vfs_op_tuple commit_ops [] =
{
{SMB_VFS_OP(commit_open),
@@ -177,6 +345,8 @@ static vfs_op_tuple commit_ops [] =
SMB_VFS_OP_PWRITE, SMB_VFS_LAYER_TRANSPARENT},
{SMB_VFS_OP(commit_connect),
SMB_VFS_OP_CONNECT, SMB_VFS_LAYER_TRANSPARENT},
+ {SMB_VFS_OP(commit_ftruncate),
+ SMB_VFS_OP_FTRUNCATE, SMB_VFS_LAYER_TRANSPARENT},
{SMB_VFS_OP(NULL), SMB_VFS_OP_NOOP, SMB_VFS_LAYER_NOOP}
};
-------------- next part --------------
> - combined: if the client doesn't declare a file size, behavior is
> the same as "strict". If a file size is declared, behaves like
> "predict"
>
> I have tested the patch with 3.0.26a, and as far as I can tell it
> behaves as expected.
>
> Unfortunately, I can't add this module to the RHEL4 and RHEL5 setups
> we have at work, due to the changes in the VFS API... I'm missing
> the ability to add extensions to the files_struct . So now I'm
> looking how to backport it to our needs.. any guides for this task?
You'd have to backport the extensions API and the corresponding
changes to the VFS layer as well. IIRC there's 2 or 3 svn revisions
that add what you need.
--
James Peach | jpeach at samba.org
More information about the samba-technical
mailing list