kern/92040: mmap/cp fails on small file in UDF

[Giorgos Keramidas]

The following reply was made to PR kern/92040; it has been noted by GNATS.

From: Giorgos Keramidas <keramida at freebsd.org>
To: Nate Eldredge <nge at cs.hmc.edu>
Cc: bug-followup at 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 at 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 at vulcan:~/bugs/udf$ mkdir dir
 > nate at vulcan:~/bugs/udf$ cat >dir/hello
 > hello world
 > nate at 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; ;
 %%%