I have attached a patch that supports a new "--direct-write" option. The result of using this option is to write directly to the destination files, instead of a temporary file first. The reason this patch is needed is for rsyncing to a device where the device is full or nearly full. Say that I am writing to a device that has 1 Meg free, and a 2 meg file on that device is out of date. Rsync will first attempt to write a new temp file, and fail, SIGUSR1'ing itself, and outputting "Error 20". Specifically, I am writing a linux root fs to a 32Meg compact flash, and libc needs to be updated, and rsync fails. This patch simply sets fnametmp to fname. Two issues with the patch, that hopefully developers can answer: - In direct-write mode, I open without O_EXCL, as the file likely does exist. Should the destination file be deleted instead? (I do not know what exactly the race condition is) - There is a section after the assignment of fnametmp, and before the open that does do_mktemp, then receive_data. What is the purpose of this part? I skip it for direct-write, and it works, but what do I know? -don -------------- next part -------------- Only in rsync-2.4.6-direct-write/lib: dummy diff -ru rsync-2.4.6/options.c rsync-2.4.6-direct-write/options.c --- rsync-2.4.6/options.c Tue Sep 5 19:46:43 2000 +++ rsync-2.4.6-direct-write/options.c Sun Nov 11 10:40:01 2001 @@ -22,6 +22,7 @@ int make_backups = 0; +int direct_write = 0; int whole_file = 0; int copy_links = 0; int preserve_links = 0; @@ -147,6 +148,7 @@ rprintf(F," --ignore-errors delete even if there are IO errors\n"); rprintf(F," --max-delete=NUM don't delete more than NUM files\n"); rprintf(F," --partial keep partially transferred files\n"); + rprintf(F," --direct-write write directly to the destination files\n"); rprintf(F," --force force deletion of directories even if not empty\n"); rprintf(F," --numeric-ids don't map uid/gid values by user/group name\n"); rprintf(F," --timeout=TIME set IO timeout in seconds\n"); @@ -188,7 +190,7 @@ OPT_LOG_FORMAT, OPT_PASSWORD_FILE, OPT_SIZE_ONLY, OPT_ADDRESS, OPT_DELETE_AFTER, OPT_EXISTING, OPT_MAX_DELETE, OPT_BACKUP_DIR, OPT_IGNORE_ERRORS, OPT_BWLIMIT, OPT_BLOCKING_IO, - OPT_MODIFY_WINDOW}; + OPT_MODIFY_WINDOW, OPT_DIRECT_WRITE}; static char *short_options = "oblLWHpguDCtcahvqrRIxnSe:B:T:zP"; @@ -227,6 +229,7 @@ {"perms", 0, 0, 'p'}, {"links", 0, 0, 'l'}, {"copy-links", 0, 0, 'L'}, + {"direct-write", 0, 0, OPT_DIRECT_WRITE}, {"copy-unsafe-links", 0, 0, OPT_COPY_UNSAFE_LINKS}, {"safe-links", 0, 0, OPT_SAFE_LINKS}, {"whole-file", 0, 0, 'W'}, @@ -400,6 +403,10 @@ safe_symlinks=1; break; + case OPT_DIRECT_WRITE: + direct_write = 1; + break; + case 'h': usage(FINFO); exit_cleanup(0); @@ -554,6 +561,8 @@ keep_partial = 1; break; + + case OPT_IGNORE_ERRORS: ignore_errors = 1; break; @@ -691,6 +700,9 @@ slprintf(mdelete,sizeof(mdelete),"--max-delete=%d",max_delete); args[ac++] = mdelete; } + + if (direct_write) + args[ac++] = "--direct-write"; if (io_timeout) { slprintf(iotime,sizeof(iotime),"--timeout=%d",io_timeout); diff -ru rsync-2.4.6/receiver.c rsync-2.4.6-direct-write/receiver.c --- rsync-2.4.6/receiver.c Thu Mar 30 06:23:03 2000 +++ rsync-2.4.6-direct-write/receiver.c Sun Nov 11 11:14:43 2001 @@ -34,6 +34,7 @@ extern char *tmpdir; extern char *compare_dest; extern int make_backups; +extern int direct_write; extern char *backup_suffix; static struct delete_list { @@ -302,7 +303,8 @@ int fd1,fd2; STRUCT_STAT st; char *fname; - char fnametmp[MAXPATHLEN]; + char *fnametmp; + char fnametmpbuf[MAXPATHLEN]; char *fnamecmp; char fnamecmpbuf[MAXPATHLEN]; struct map_struct *buf; @@ -314,6 +316,7 @@ extern int preserve_perms; extern int delete_after; struct stats initial_stats; + int write_flags; if (verbose > 2) { rprintf(FINFO,"recv_files(%d) starting\n",flist->count); @@ -404,22 +407,29 @@ buf = NULL; } - if (!get_tmpname(fnametmp,fname)) { - if (buf) unmap_file(buf); - if (fd1 != -1) close(fd1); - continue; - } + if(direct_write) { + fnametmp = fname; + write_flags = O_WRONLY|O_CREAT; + } else { + fnametmp = fnametmpbuf; + if (!get_tmpname(fnametmp,fname)) { + if (buf) unmap_file(buf); + if (fd1 != -1) close(fd1); + continue; + } + write_flags = O_WRONLY|O_CREAT|O_EXCL; /* mktemp is deliberately used here instead of mkstemp. because O_EXCL is used on the open, the race condition is not a problem or a security hole, and we want to control the access permissions on the created file. */ - if (NULL == do_mktemp(fnametmp)) { - rprintf(FERROR,"mktemp %s failed\n",fnametmp); - receive_data(f_in,buf,-1,NULL,file->length); - if (buf) unmap_file(buf); - if (fd1 != -1) close(fd1); - continue; + if (NULL == do_mktemp(fnametmp)) { + rprintf(FERROR,"mktemp %s failed\n",fnametmp); + receive_data(f_in,buf,-1,NULL,file->length); + if (buf) unmap_file(buf); + if (fd1 != -1) close(fd1); + continue; + } } /* we initially set the perms without the @@ -428,7 +438,7 @@ the lchown. Thanks to snabb@epipe.fi for pointing this out. We also set it initially without group access because of a similar race condition. */ - fd2 = do_open(fnametmp,O_WRONLY|O_CREAT|O_EXCL, + fd2 = do_open(fnametmp,write_flags, file->mode & INITACCESSPERMS); /* in most cases parent directories will already exist @@ -436,7 +446,7 @@ transferred, but that may not be the case with -R */ if (fd2 == -1 && relative_paths && errno == ENOENT && create_directory_path(fnametmp) == 0) { - fd2 = do_open(fnametmp,O_WRONLY|O_CREAT|O_EXCL, + fd2 = do_open(fnametmp,write_flags, file->mode & INITACCESSPERMS); } if (fd2 == -1) {
Oh boy, I think you're getting into quite a can of worms there. At a minimum this option should imply the --partial option because if the operation is aborted the file will be left partially transferred. Note that if you're trying to use the rsync rolling checksum algorithm to minimize bandwidth that if a transfer is interrupted all the previous data will be lost so there will be less data to compare against when the transfer is retried. Next, keep in mind that the receiving side of an rsync transfer uses two independent processes, one for generating checksums and one for creating files. I'm not knowledgable enough to know whether or not the creating files operation is guaranteed to begin only after the checksum generation is completed on each file, but if it isn't then overwriting a file could be a big problem. Have you tried transferring any very large files? I'm surprised you didn't need to do anything to finish_transfer to keep the robust_rename from returning an error. The last thing that comes to mind is that overwriting files that are open by another process, such as a running executable, can be a problem. An unlink/rename works much better for that. I'm sure there are more issues too. - Dave Dykstra On Mon, Nov 12, 2001 at 10:12:54AM -0800, Don Mahurin wrote:> I have attached a patch that supports a new "--direct-write" option. > > The result of using this option is to write directly to the destination > files, instead of a temporary file first. > > The reason this patch is needed is for rsyncing to a device where the > device is full or nearly full. > > Say that I am writing to a device that has 1 Meg free, and a 2 meg file > on that device is out of date. > Rsync will first attempt to write a new temp file, and fail, SIGUSR1'ing > itself, and outputting "Error 20". > > Specifically, I am writing a linux root fs to a 32Meg compact flash, and > libc needs to be updated, and rsync fails. > > This patch simply sets fnametmp to fname. > > Two issues with the patch, that hopefully developers can answer: > > - In direct-write mode, I open without O_EXCL, as the file likely does > exist. > Should the destination file be deleted instead? (I do not know what > exactly the race condition is) > > - There is a section after the assignment of fnametmp, and before the > open that does do_mktemp, then > receive_data. What is the purpose of this part? I skip it for > direct-write, and it works, but what do I know? > > > -don> Only in rsync-2.4.6-direct-write/lib: dummy > diff -ru rsync-2.4.6/options.c rsync-2.4.6-direct-write/options.c > --- rsync-2.4.6/options.c Tue Sep 5 19:46:43 2000 > +++ rsync-2.4.6-direct-write/options.c Sun Nov 11 10:40:01 2001 > @@ -22,6 +22,7 @@ > > > int make_backups = 0; > +int direct_write = 0; > int whole_file = 0; > int copy_links = 0; > int preserve_links = 0; > @@ -147,6 +148,7 @@ > rprintf(F," --ignore-errors delete even if there are IO errors\n"); > rprintf(F," --max-delete=NUM don't delete more than NUM files\n"); > rprintf(F," --partial keep partially transferred files\n"); > + rprintf(F," --direct-write write directly to the destination files\n"); > rprintf(F," --force force deletion of directories even if not empty\n"); > rprintf(F," --numeric-ids don't map uid/gid values by user/group name\n"); > rprintf(F," --timeout=TIME set IO timeout in seconds\n"); > @@ -188,7 +190,7 @@ > OPT_LOG_FORMAT, OPT_PASSWORD_FILE, OPT_SIZE_ONLY, OPT_ADDRESS, > OPT_DELETE_AFTER, OPT_EXISTING, OPT_MAX_DELETE, OPT_BACKUP_DIR, > OPT_IGNORE_ERRORS, OPT_BWLIMIT, OPT_BLOCKING_IO, > - OPT_MODIFY_WINDOW}; > + OPT_MODIFY_WINDOW, OPT_DIRECT_WRITE}; > > static char *short_options = "oblLWHpguDCtcahvqrRIxnSe:B:T:zP"; > > @@ -227,6 +229,7 @@ > {"perms", 0, 0, 'p'}, > {"links", 0, 0, 'l'}, > {"copy-links", 0, 0, 'L'}, > + {"direct-write", 0, 0, OPT_DIRECT_WRITE}, > {"copy-unsafe-links", 0, 0, OPT_COPY_UNSAFE_LINKS}, > {"safe-links", 0, 0, OPT_SAFE_LINKS}, > {"whole-file", 0, 0, 'W'}, > @@ -400,6 +403,10 @@ > safe_symlinks=1; > break; > > + case OPT_DIRECT_WRITE: > + direct_write = 1; > + break; > + > case 'h': > usage(FINFO); > exit_cleanup(0); > @@ -554,6 +561,8 @@ > keep_partial = 1; > break; > > + > + > case OPT_IGNORE_ERRORS: > ignore_errors = 1; > break; > @@ -691,6 +700,9 @@ > slprintf(mdelete,sizeof(mdelete),"--max-delete=%d",max_delete); > args[ac++] = mdelete; > } > + > + if (direct_write) > + args[ac++] = "--direct-write"; > > if (io_timeout) { > slprintf(iotime,sizeof(iotime),"--timeout=%d",io_timeout); > diff -ru rsync-2.4.6/receiver.c rsync-2.4.6-direct-write/receiver.c > --- rsync-2.4.6/receiver.c Thu Mar 30 06:23:03 2000 > +++ rsync-2.4.6-direct-write/receiver.c Sun Nov 11 11:14:43 2001 > @@ -34,6 +34,7 @@ > extern char *tmpdir; > extern char *compare_dest; > extern int make_backups; > +extern int direct_write; > extern char *backup_suffix; > > static struct delete_list { > @@ -302,7 +303,8 @@ > int fd1,fd2; > STRUCT_STAT st; > char *fname; > - char fnametmp[MAXPATHLEN]; > + char *fnametmp; > + char fnametmpbuf[MAXPATHLEN]; > char *fnamecmp; > char fnamecmpbuf[MAXPATHLEN]; > struct map_struct *buf; > @@ -314,6 +316,7 @@ > extern int preserve_perms; > extern int delete_after; > struct stats initial_stats; > + int write_flags; > > if (verbose > 2) { > rprintf(FINFO,"recv_files(%d) starting\n",flist->count); > @@ -404,22 +407,29 @@ > buf = NULL; > } > > - if (!get_tmpname(fnametmp,fname)) { > - if (buf) unmap_file(buf); > - if (fd1 != -1) close(fd1); > - continue; > - } > + if(direct_write) { > + fnametmp = fname; > + write_flags = O_WRONLY|O_CREAT; > + } else { > + fnametmp = fnametmpbuf; > + if (!get_tmpname(fnametmp,fname)) { > + if (buf) unmap_file(buf); > + if (fd1 != -1) close(fd1); > + continue; > + } > + write_flags = O_WRONLY|O_CREAT|O_EXCL; > > /* mktemp is deliberately used here instead of mkstemp. > because O_EXCL is used on the open, the race condition > is not a problem or a security hole, and we want to > control the access permissions on the created file. */ > - if (NULL == do_mktemp(fnametmp)) { > - rprintf(FERROR,"mktemp %s failed\n",fnametmp); > - receive_data(f_in,buf,-1,NULL,file->length); > - if (buf) unmap_file(buf); > - if (fd1 != -1) close(fd1); > - continue; > + if (NULL == do_mktemp(fnametmp)) { > + rprintf(FERROR,"mktemp %s failed\n",fnametmp); > + receive_data(f_in,buf,-1,NULL,file->length); > + if (buf) unmap_file(buf); > + if (fd1 != -1) close(fd1); > + continue; > + } > } > > /* we initially set the perms without the > @@ -428,7 +438,7 @@ > the lchown. Thanks to snabb@epipe.fi for pointing > this out. We also set it initially without group > access because of a similar race condition. */ > - fd2 = do_open(fnametmp,O_WRONLY|O_CREAT|O_EXCL, > + fd2 = do_open(fnametmp,write_flags, > file->mode & INITACCESSPERMS); > > /* in most cases parent directories will already exist > @@ -436,7 +446,7 @@ > transferred, but that may not be the case with -R */ > if (fd2 == -1 && relative_paths && errno == ENOENT && > create_directory_path(fnametmp) == 0) { > - fd2 = do_open(fnametmp,O_WRONLY|O_CREAT|O_EXCL, > + fd2 = do_open(fnametmp,write_flags, > file->mode & INITACCESSPERMS); > } > if (fd2 == -1) {
Don Mahurin [dmahurin@berkeley.innomedia.com] writes:> Perhaps, all that I need is a "--delete-before-update" option that > just unlinks the file before it starts to write the temp file. Then > we avoid the possible issues that you raised. I can still see a > case where --direct-write may be useful (read-write file in a > read-only dir), but this is probably not a common situation, and I > don't want to tackle those issues yet.But that would defeat the rsync algorithm, since it is reading from the original file while writing the temp file (in order to copy over unchanged blocks to optimize the transfer). So you need the original while constructing the new version. This is also true for directly overwriting the original file, since the algorithm depends on being able to reference blocks from that original file at any point in the transfer. By overwriting it you risk corruption of the final file if rsync wants to reuse a block from early in the file late in the transfer, which you could have already overwritten. It doesn't appear that your original patch takes this into account (unless you are also forcing -W and disabling the rsync protocol if your new option is used). In this respect, rsync is a tradeoff of local CPU and disk space on either side of the connection in order to optimize the traffic between the two. It needs that space (and computation time) to determine how to most efficiently communicate the file differences. I have a feeling that trying to reduce the disk space needs is going to be counter-productive since it goes against the fundamental tradeoffs inherent in rsync. -- David /-----------------------------------------------------------------------\ \ David Bolen \ E-mail: db3l@fitlinxx.com / | FitLinxx, Inc. \ Phone: (203) 708-5192 | / 860 Canal Street, Stamford, CT 06902 \ Fax: (203) 316-5150 \ \-----------------------------------------------------------------------/