Date: Mon, 23 Jan 2006 03:20:13 GMT From: Giorgos Keramidas <keramida@freebsd.org> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/92040: mmap/cp fails on small file in UDF Message-ID: <200601230320.k0N3KDbp053381@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/92040; it has been noted by GNATS.
From: Giorgos Keramidas <keramida@freebsd.org>
To: Nate Eldredge <nge@cs.hmc.edu>
Cc: bug-followup@FreeBSD.org
Subject: Re: kern/92040: mmap/cp fails on small file in UDF
Date: Mon, 23 Jan 2006 05:15:47 +0200
On 2006-01-19 22:34, Nate Eldredge <nge@cs.hmc.edu> wrote:
> If you try to copy a small file from a UDF filesystem, cp fails with EINVAL.
>
> The underlying cause is that cp attempts to copy small files using mmap,
> and mmap fails in this case. I don't know whether cp is calling it wrong,
> or mmap is simply unimplemented for udf, or if there is some other reason
> for the failure.
>
> In any case, it is probably a good idea for cp to fall back to read() if
> mmap doesn't work. It might also be useful to be able to explicitly
> tell it to use read(). For example, I don't know what the mmap approach
> will do in the presence of I/O errors. I'm worried that it might silently
> make a corrupt copy of the file.
>
> This is on amd64.
>
> Perhaps this bug should be cross-filed in category bin, if that is possible.
> I don't know how to do that.
>
> >How-To-Repeat:
> nate@vulcan:~/bugs/udf$ mkdir dir
> nate@vulcan:~/bugs/udf$ cat >dir/hello
> hello world
> nate@vulcan:~/bugs/udf$ mkisofs -o img -udf dir/
> ...
> vulcan# mdconfig -a -t vnode -f img
> md0
> vulcan# mount_udf /dev/md0 /mnt/md0
> vulcan# ls -l /mnt/md0
> total 0
> -r--r--r-- 1 root wheel 12 Jan 20 22:10 hello
> vulcan# cat /mnt/md0/hello
> hello world
> vulcan# cp /mnt/md0/hello .
> cp: /mnt/md0/hello: Invalid argument
The following patch adds a `retry' variable in utils.c, which is set by
default to true. If mmap() succeeds, then `retry' is reset to false, to
avoid copying some blocks with mmap() and others with read/write.
It seems to work fine here...
$ mount | grep /mnt
/dev/md1 on /mnt (udf, local, read-only)
$ ls -l /mnt
total 0
-r--r--r-- 1 root wheel - 12 Jan 24 05:03 hello
$ ./cp /mnt/hello .
$ ls -ld hello
-r--r--r-- 1 keramida keramida - 12 Jan 23 05:12 hello
$ cmp /mnt/hello hello ; echo $?
0
$
Removing the need for rval = 1 when mmap() fails, means we can revert
the condition of the following if statement and reduce the indentation
level too:
%%%
Index: utils.c
===================================================================
RCS file: /home/ncvs/src/bin/cp/utils.c,v
retrieving revision 1.46
diff -u -r1.46 utils.c
--- utils.c 5 Sep 2005 04:36:08 -0000 1.46
+++ utils.c 23 Jan 2006 03:09:33 -0000
@@ -61,7 +61,7 @@
{
static char buf[MAXBSIZE];
struct stat *fs;
- int ch, checkch, from_fd, rcount, rval, to_fd;
+ int ch, checkch, from_fd, rcount, retry, rval, to_fd;
ssize_t wcount;
size_t wresid;
size_t wtotal;
@@ -125,6 +125,7 @@
}
rval = 0;
+ retry = 1; /* Retry copying if mmap() fails. */
/*
* Mmap and write if less than 8M (the limit is so we don't totally
@@ -133,41 +134,37 @@
*/
#ifdef VM_AND_BUFFER_CACHE_SYNCHRONIZED
if (S_ISREG(fs->st_mode) && fs->st_size > 0 &&
- fs->st_size <= 8 * 1048576) {
- if ((p = mmap(NULL, (size_t)fs->st_size, PROT_READ,
- MAP_SHARED, from_fd, (off_t)0)) == MAP_FAILED) {
+ fs->st_size <= 8 * 1048576 &&
+ (p = mmap(NULL, (size_t)fs->st_size, PROT_READ,
+ MAP_SHARED, from_fd, (off_t)0)) != MAP_FAILED) {
+ retry = 0;
+ wtotal = 0;
+ for (bufp = p, wresid = fs->st_size; ;
+ bufp += wcount, wresid -= (size_t)wcount) {
+ wcount = write(to_fd, bufp, wresid);
+ wtotal += wcount;
+ if (info) {
+ info = 0;
+ (void)fprintf(stderr,
+ "%s -> %s %3d%%\n",
+ entp->fts_path, to.p_path,
+ cp_pct(wtotal, fs->st_size));
+ }
+ if (wcount >= (ssize_t)wresid || wcount <= 0)
+ break;
+ }
+ if (wcount != (ssize_t)wresid) {
+ warn("%s", to.p_path);
+ rval = 1;
+ }
+ /* Some systems don't unmap on close(2). */
+ if (munmap(p, fs->st_size) < 0) {
warn("%s", entp->fts_path);
rval = 1;
- } else {
- wtotal = 0;
- for (bufp = p, wresid = fs->st_size; ;
- bufp += wcount, wresid -= (size_t)wcount) {
- wcount = write(to_fd, bufp, wresid);
- wtotal += wcount;
- if (info) {
- info = 0;
- (void)fprintf(stderr,
- "%s -> %s %3d%%\n",
- entp->fts_path, to.p_path,
- cp_pct(wtotal, fs->st_size));
-
- }
- if (wcount >= (ssize_t)wresid || wcount <= 0)
- break;
- }
- if (wcount != (ssize_t)wresid) {
- warn("%s", to.p_path);
- rval = 1;
- }
- /* Some systems don't unmap on close(2). */
- if (munmap(p, fs->st_size) < 0) {
- warn("%s", entp->fts_path);
- rval = 1;
- }
}
- } else
+ }
#endif
- {
+ if (retry != 0) {
wtotal = 0;
while ((rcount = read(from_fd, buf, MAXBSIZE)) > 0) {
for (bufp = buf, wresid = rcount; ;
%%%
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200601230320.k0N3KDbp053381>