[PATCH] add pread operation to vfs layer

Stefan Metzmacher metze at metzemix.de
Wed Dec 17 05:56:20 GMT 2003


James Peach wrote:

 >On Tue, 16 Dec 2003 15:27:47 -0800
 >"Cole, Timothy D." <t.cole at ngc.com> wrote:
 >
 >>I wrote:
 >>
 >>>Would it be feasible to cache the effective and actual file
 >>>offsets for a
 >>>file descriptor, to reduce the number of seeks needed for sequential
 >>>pwrites/preads?
 >>
 >>Thinking about it, those offsets would likely have to be stored in the
 >>files_struct -- that's okay provided an fd with a files_struct attached
 >>can only ever be read/written/lsought via that single files_struct.
 >>
 >>Is that a safe assumption to make, though?
 >
 >
 >This seems quite complicated and difficult to test. There is already 
pos and
 >position_information in files_struct, and adding a third player into this
 >isn't going to help the clarity of this code.
 >
 >Another different option is to test for p* at runtime, which would allow
 >3rd-party vfs modules to add their owm p* ops (perhaps implementing your
 >caching idea). This patch (untested and cumulative wrt my p* one) hacks up
 >the vfs to allow operatios to be missing ...
 >
 >--
 >James Peach | jpeach at sgi.com | SGI Australian Software Group
 >I don't speak for SGI.
 >
 >
 >===========================================================================
 >source/include/vfs_macros.h
 >===========================================================================
 >
 >--- /usr/tmp/TmpDir.1294301-0/src/source/include/vfs_macros.h_1.5    
Wed Dec 17 13:38:10 2003
 >+++ source/include/vfs_macros.h    Wed Dec 17 13:33:53 2003
 >@@ -27,6 +27,12 @@
 >  (Fixes should go also into the vfs_opaque_* and vfs_next_* macros!)
 > ********************************************************************/
 >
 >+extern int vfswrap_enosys(struct vfs_handle_struct *, struct 
files_struct *, ...);
 >+
 >+/* Define as operation as not present if there is no opaque version 
of it. */
 >+#define SMB_VFS_OP_MISSING(conn, op) \
 >+        (((void **)&(conn)->vfs_opaque.ops)[op] == (void 
*)vfswrap_enosys)
 >+
 > /* Disk operations */   
 > #define SMB_VFS_CONNECT(conn, service, user) 
((conn)->vfs.ops.connect((conn)->vfs.handles.connect, (conn), (service), 
(user)))
 > #define SMB_VFS_DISCONNECT(conn) 
((conn)->vfs.ops.disconnect((conn)->vfs.handles.disconnect, (conn)))
 >
 >===========================================================================
 >source/smbd/fileio.c
 >===========================================================================
 >
 >--- /usr/tmp/TmpDir.1294301-0/src/source/smbd/fileio.c_1.5    Wed Dec 
17 13:38:10 2003
 >+++ source/smbd/fileio.c    Wed Dec 17 13:36:00 2003
 >@@ -95,22 +95,20 @@
 >
 >     flush_write_cache(fsp, READ_FLUSH);
 >
 >-#if !defined(HAVE_PREAD) && !defined(HAVE_PREAD64)
 >-    if (seek_file(fsp,pos) == -1) {
 >+    if (SMB_VFS_OP_MISSING(fsp->conn, SMB_VFS_OP_PREAD) &&
 >+        seek_file(fsp,pos) == -1) {
 >         DEBUG(3,("read_file: Failed to seek to %.0f\n",(double)pos));
 >         return(ret);
 >     }
 >-#endif
 >
 >     if (n > 0) {
 > #ifdef DMF_FIX
 >         int numretries = 3;
 > tryagain:
 >-#if defined(HAVE_PREAD) || defined(HAVE_PREAD64)
 >-        readret = SMB_VFS_PREAD(fsp,fsp->fd,data,n,pos);
 >-#else
 >-        readret = SMB_VFS_READ(fsp,fsp->fd,data,n);
 >-#endif
 >+        readret = SMB_VFS_OP_MISSING(&fsp->conn, SMB_VFS_OP_PREAD) ?
 >+                SMB_VFS_READ(fsp,fsp->fd,data,n) :
 >+                SMB_VFS_PREAD(fsp,fsp->fd,data,n,pos);
 >+
 >         if (readret == -1) {
 >             if ((errno == EAGAIN) && numretries) {
 >                 DEBUG(3,("read_file EAGAIN retry in 10 seconds\n"));
 >@@ -121,11 +119,10 @@
 >             return -1;
 >         }
 > #else /* NO DMF fix. */
 >-#if defined(HAVE_PREAD) || defined(HAVE_PREAD64)
 >-        readret = SMB_VFS_PREAD(fsp,fsp->fd,data,n,pos);
 >-#else
 >-        readret = SMB_VFS_READ(fsp,fsp->fd,data,n);
 >-#endif
 >+        readret = SMB_VFS_OP_MISSING(fsp->conn, SMB_VFS_OP_PREAD) ?
 >+                SMB_VFS_READ(fsp,fsp->fd,data,n) :
 >+                SMB_VFS_PREAD(fsp,fsp->fd,data,n,pos);
 >+
 >         if (readret == -1)
 >             return -1;
 > #endif
 >@@ -153,17 +150,17 @@
 > {
 >     ssize_t ret;
 >
 >-#if defined(HAVE_PREAD) || defined(HAVE_PREAD64)
 >-        if (pos == -1)
 >-                ret = vfs_write_data(fsp, data, n);
 >-        else
 >-                ret = vfs_pwrite_data(fsp, data, n, pos);
 >-#else
 >-    if ((pos != -1) && (seek_file(fsp,pos) == -1))
 >-        return -1;
 >+    if (SMB_VFS_OP_MISSING(fsp->conn, SMB_VFS_OP_PWRITE)) {
 >+        if ((pos != -1) && (seek_file(fsp,pos) == -1))
 >+            return -1;
 >
 >-    ret = vfs_write_data(fsp,data,n);
 >-#endif
 >+        ret = vfs_write_data(fsp,data,n);
 >+    } else {
 >+        if (pos == -1)
 >+            ret = vfs_write_data(fsp, data, n);
 >+        else
 >+            ret = vfs_pwrite_data(fsp, data, n, pos);
 >+    }
 >
 >     DEBUG(10,("real_write_file (%s): pos = %.0f, size = %lu, returned 
%ld\n",
 >         fsp->fsp_name, (double)pos, (unsigned long)n, (long)ret ));
 >
 >===========================================================================
 >source/smbd/vfs-wrap.c
 >===========================================================================
 >
 >--- /usr/tmp/TmpDir.1294301-0/src/source/smbd/vfs-wrap.c_1.6    Wed 
Dec 17 13:38:10 2003
 >+++ source/smbd/vfs-wrap.c    Wed Dec 17 12:56:16 2003
 >@@ -562,6 +562,12 @@
 >     return 0;
 > }
 >
 >+int vfswrap_enosys(struct vfs_handle_struct * handle, struct 
files_struct * fsp, ...)
 >+{
 >+        errno = ENOSYS;
 >+        return -1;
 >+}
 >+
 > int vfswrap_ftruncate(vfs_handle_struct *handle, files_struct *fsp, 
int fd, SMB_OFF_T len)
 > {
 >     int result = -1;
 >
 >===========================================================================
 >source/smbd/vfs.c
 >===========================================================================
 >
 >--- /usr/tmp/TmpDir.1294301-0/src/source/smbd/vfs.c_1.10    Wed Dec 17 
13:38:10 2003
 >+++ source/smbd/vfs.c    Wed Dec 17 13:36:56 2003
 >@@ -46,6 +46,7 @@
 >    very important.  They must be in the same order as defined in
 >    vfs.h.  Change at your own peril. */
 >
 >+
 > static struct vfs_ops default_vfs = {
 >
 >     {
 >@@ -71,9 +72,17 @@
 >         vfswrap_open,
 >         vfswrap_close,
 >         vfswrap_read,
 >+#if defined(HAVE_PREAD) || defined(HAVE_PREAD64)
 >         vfswrap_pread,
 >+#else
 >+                vfswrap_enosys,
 >+#endif
 >         vfswrap_write,
 >+#if defined(HAVE_PWRITE) || defined(HAVE_PWRITE64)
 >         vfswrap_pwrite,
 >+#else
 >+                vfswrap_enosys,
 >+#endif
 >         vfswrap_lseek,
 >         vfswrap_sendfile,
 >         vfswrap_rename,
 >
I like the first patch more...

It's ugly to test if a function pointer has a specefic value.
What is when e.g. a  database  opaque  vfs module overloads the pread fn 
with

example_pread(vfs_handle_struct *handle,files_struct *f, ...)
{
         errno =ENOSYS;
          return -1;
}

(This is what such a module should do)

So just try SMB_VFS_PREAD and if it fails with ENOSYS try read.
So no if def's are needed

if (((readret=SMB_VFS_PREAD(fsp,fsp->fd,data,n,pos))==-1)&&errno==ENOSYS)
    readret = SMB_VFS_READ(fsp,fsp->fd,data,n);

}
-- 

metze

-------------------------------------------
Stefan (metze) Metzmacher <metze at metzemix.de>




More information about the samba-technical mailing list