Bug 156933 - [zfs] ZFS receive after read on readonly=on filesystem is corrupted without warning
Summary: [zfs] ZFS receive after read on readonly=on filesystem is corrupted without w...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 8.2-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-fs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-10 18:20 UTC by D. Secret
Modified: 2011-09-02 09:23 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description D. Secret 2011-05-10 18:20:10 UTC
If you read from a readonly=on ZFS filesystem before receiving a ZFS -i send for new snapshots it will receive the new snapshot and have a corrupted filesystem(that ZFS scrub does not detect!)  The one file on the filesystem I've included will show this corruption.  It should simply be a file of consecutive numbers, yet the last like will have some messed up data.

The ZFS 'images' were created as such:
13.zfs: A snapshot export of the filesystem
 -=- Append "14" to the replicate.dat file
 -=- Create snapshot @14
diff.zfs : a -i snapshot between @13 and @14.  It properly re-creates the file assuming you haven't read from the filesystem.  Note: Reading from the filesystem in this manner is present in some FAQ's about ZFS replication.

Fix: 

I do not have a fix, but I do have what I believe is a 100% reproduction so I imagine a fix shouldn't be too difficult.   I would look for what change is being made to the ZFS filesystem while readonly=on is set and a file is read...It should be none, as far as I know.
How-To-Repeat: 1. Create a new zfs filesystem
zfs set readonly=on (my new filesystem)
cat zimg/13.zfs | zfs receive -F (my new filesystem)

OPTIONAL: cat (my new filesystem)/replicate.dat    
[This will cause the corruption if you do it]

cat zimg/diff.zfs | zfs receive (my new filesystem)

Now inspect (my new filesystem)/replicate.dat .. 

BUG file SHA1 is b3af2ae0f05cceb7efbcfc747750f56f53e4a69e
Proper file SHA1 is 42f9e9bf4534b43b57faaed39194115bf3517898

You'll notice it'll be different depending on if you performed the "optional" step.  I'm pretty sure this isn't proper.  I've verified that the atime of the file isn't changing, but there must be some subtle change going on that isn't being caught.  Also the zfs receive is allowed in both cases, but in one case the file is corrupted (vi shows 3x ^@ at the end, rather than the expected 14\n)

I can offer you access to a machine if you have difficulty reproducing this issue.  My scripts & sample filesystems are at:
http://sov.L93.com/bug.tgz
SHA1: 62b700d95096474ed7f3ae0b9249f9ec77e9a747
MD5: 9b9aa38b322cedd3e1223fc50f18a55b

There are no executables, just a very simple 5 or so line (rather basic) script to automate things if you choose to use them.  Please note it will destroy filesystem "z0/rdata_rcv" if you happen to have a ZFS filesystem by that name. ;)

A shar archive of the 11k TGZ is 711k, beyond what i can attach to this.  Hopefully the web link suffices.
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2011-05-10 20:30:17 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-fs

Over to maintainer(s).
Comment 2 Martin Matuska freebsd_committer freebsd_triage 2011-07-18 12:52:21 UTC
I am unable to reproduce this on ZFS v28.
Can you give me detailed instructions and necessary files?

-- 
Martin Matuska
FreeBSD committer
http://blog.vx.sk
Comment 3 dfilter service freebsd_committer freebsd_triage 2011-08-25 09:17:55 UTC
Author: mm
Date: Thu Aug 25 08:17:39 2011
New Revision: 225166
URL: http://svn.freebsd.org/changeset/base/225166

Log:
  Generalize ffs_pages_remove() into vn_pages_remove().
  
  Remove mapped pages for all dataset vnodes in zfs_rezget() using
  new vn_pages_remove() to fix mmapped files changed by
  zfs rollback or zfs receive -F.
  
  PR:		kern/160035, kern/156933
  Reviewed by:	kib, pjd
  Approved by:	re (kib)
  MFC after:	1 week

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
  head/sys/kern/vfs_vnops.c
  head/sys/sys/vnode.h
  head/sys/ufs/ffs/ffs_extern.h
  head/sys/ufs/ffs/ffs_inode.c
  head/sys/ufs/ffs/ffs_softdep.c

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c	Thu Aug 25 07:28:07 2011	(r225165)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c	Thu Aug 25 08:17:39 2011	(r225166)
@@ -1273,6 +1273,7 @@ zfs_rezget(znode_t *zp)
 	zfsvfs_t *zfsvfs = zp->z_zfsvfs;
 	dmu_object_info_t doi;
 	dmu_buf_t *db;
+	vnode_t *vp;
 	uint64_t obj_num = zp->z_id;
 	uint64_t mode, size;
 	sa_bulk_attr_t bulk[8];
@@ -1348,8 +1349,9 @@ zfs_rezget(znode_t *zp)
 	 * that for example regular file was replaced with directory
 	 * which has the same object number.
 	 */
-	if (ZTOV(zp) != NULL &&
-	    ZTOV(zp)->v_type != IFTOVT((mode_t)zp->z_mode)) {
+	vp = ZTOV(zp);
+	if (vp != NULL &&
+	    vp->v_type != IFTOVT((mode_t)zp->z_mode)) {
 		zfs_znode_dmu_fini(zp);
 		ZFS_OBJ_HOLD_EXIT(zfsvfs, obj_num);
 		return (EIO);
@@ -1357,8 +1359,11 @@ zfs_rezget(znode_t *zp)
 
 	zp->z_unlinked = (zp->z_links == 0);
 	zp->z_blksz = doi.doi_data_block_size;
-	if (zp->z_size != size && ZTOV(zp) != NULL)
-		vnode_pager_setsize(ZTOV(zp), zp->z_size);
+	if (vp != NULL) {
+		vn_pages_remove(vp, 0, 0);
+		if (zp->z_size != size)
+			vnode_pager_setsize(vp, zp->z_size);
+	}
 
 	ZFS_OBJ_HOLD_EXIT(zfsvfs, obj_num);
 

Modified: head/sys/kern/vfs_vnops.c
==============================================================================
--- head/sys/kern/vfs_vnops.c	Thu Aug 25 07:28:07 2011	(r225165)
+++ head/sys/kern/vfs_vnops.c	Thu Aug 25 08:17:39 2011	(r225166)
@@ -64,6 +64,9 @@ __FBSDID("$FreeBSD$");
 #include <security/audit/audit.h>
 #include <security/mac/mac_framework.h>
 
+#include <vm/vm.h>
+#include <vm/vm_object.h>
+
 static fo_rdwr_t	vn_read;
 static fo_rdwr_t	vn_write;
 static fo_truncate_t	vn_truncate;
@@ -1398,3 +1401,15 @@ vn_chown(struct file *fp, uid_t uid, gid
 	VFS_UNLOCK_GIANT(vfslocked);
 	return (error);
 }
+
+void
+vn_pages_remove(struct vnode *vp, vm_pindex_t start, vm_pindex_t end)
+{
+	vm_object_t object;
+
+	if ((object = vp->v_object) == NULL)
+		return;
+	VM_OBJECT_LOCK(object);
+	vm_object_page_remove(object, start, end, 0);
+	VM_OBJECT_UNLOCK(object);
+}

Modified: head/sys/sys/vnode.h
==============================================================================
--- head/sys/sys/vnode.h	Thu Aug 25 07:28:07 2011	(r225165)
+++ head/sys/sys/vnode.h	Thu Aug 25 08:17:39 2011	(r225166)
@@ -640,6 +640,7 @@ int	_vn_lock(struct vnode *vp, int flags
 int	vn_open(struct nameidata *ndp, int *flagp, int cmode, struct file *fp);
 int	vn_open_cred(struct nameidata *ndp, int *flagp, int cmode,
 	    u_int vn_open_flags, struct ucred *cred, struct file *fp);
+void	vn_pages_remove(struct vnode *vp, vm_pindex_t start, vm_pindex_t end);
 int	vn_pollrecord(struct vnode *vp, struct thread *p, int events);
 int	vn_rdwr(enum uio_rw rw, struct vnode *vp, void *base,
 	    int len, off_t offset, enum uio_seg segflg, int ioflg,

Modified: head/sys/ufs/ffs/ffs_extern.h
==============================================================================
--- head/sys/ufs/ffs/ffs_extern.h	Thu Aug 25 07:28:07 2011	(r225165)
+++ head/sys/ufs/ffs/ffs_extern.h	Thu Aug 25 08:17:39 2011	(r225166)
@@ -79,7 +79,6 @@ int	ffs_isfreeblock(struct fs *, u_char 
 void	ffs_load_inode(struct buf *, struct inode *, struct fs *, ino_t);
 int	ffs_mountroot(void);
 void	ffs_oldfscompat_write(struct fs *, struct ufsmount *);
-void	ffs_pages_remove(struct vnode *vp, vm_pindex_t start, vm_pindex_t end);
 int	ffs_reallocblks(struct vop_reallocblks_args *);
 int	ffs_realloccg(struct inode *, ufs2_daddr_t, ufs2_daddr_t,
 	    ufs2_daddr_t, int, int, int, struct ucred *, struct buf **);

Modified: head/sys/ufs/ffs/ffs_inode.c
==============================================================================
--- head/sys/ufs/ffs/ffs_inode.c	Thu Aug 25 07:28:07 2011	(r225165)
+++ head/sys/ufs/ffs/ffs_inode.c	Thu Aug 25 08:17:39 2011	(r225166)
@@ -120,18 +120,6 @@ ffs_update(vp, waitfor)
 	}
 }
 
-void
-ffs_pages_remove(struct vnode *vp, vm_pindex_t start, vm_pindex_t end)
-{
-	vm_object_t object;
-
-	if ((object = vp->v_object) == NULL)
-		return;
-	VM_OBJECT_LOCK(object);
-	vm_object_page_remove(object, start, end, 0);
-	VM_OBJECT_UNLOCK(object);
-}
-
 #define	SINGLE	0	/* index of single indirect block */
 #define	DOUBLE	1	/* index of double indirect block */
 #define	TRIPLE	2	/* index of triple indirect block */
@@ -219,7 +207,7 @@ ffs_truncate(vp, length, flags, cred, td
 			(void) chkdq(ip, -extblocks, NOCRED, 0);
 #endif
 			vinvalbuf(vp, V_ALT, 0, 0);
-			ffs_pages_remove(vp,
+			vn_pages_remove(vp,
 			    OFF_TO_IDX(lblktosize(fs, -extblocks)), 0);
 			osize = ip->i_din2->di_extsize;
 			ip->i_din2->di_blocks -= extblocks;

Modified: head/sys/ufs/ffs/ffs_softdep.c
==============================================================================
--- head/sys/ufs/ffs/ffs_softdep.c	Thu Aug 25 07:28:07 2011	(r225165)
+++ head/sys/ufs/ffs/ffs_softdep.c	Thu Aug 25 08:17:39 2011	(r225166)
@@ -6541,7 +6541,7 @@ trunc_pages(ip, length, extblocks, flags
 	fs = ip->i_fs;
 	extend = OFF_TO_IDX(lblktosize(fs, -extblocks));
 	if ((flags & IO_EXT) != 0)
-		ffs_pages_remove(vp, extend, 0);
+		vn_pages_remove(vp, extend, 0);
 	if ((flags & IO_NORMAL) == 0)
 		return;
 	BO_LOCK(&vp->v_bufobj);
@@ -6567,7 +6567,7 @@ trunc_pages(ip, length, extblocks, flags
 		end = OFF_TO_IDX(lblktosize(fs, lbn));
 	} else
 		end = extend;
-	ffs_pages_remove(vp, OFF_TO_IDX(OFF_MAX), end);
+	vn_pages_remove(vp, OFF_TO_IDX(OFF_MAX), end);
 }
 
 /*
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 4 dfilter service freebsd_committer freebsd_triage 2011-09-02 09:19:40 UTC
Author: mm
Date: Fri Sep  2 08:19:19 2011
New Revision: 225326
URL: http://svn.freebsd.org/changeset/base/225326

Log:
  MFC r226155:
  
  Generalize ffs_pages_remove() into vn_pages_remove().
  
  Remove mapped pages for all dataset vnodes in zfs_rezget() using
  new vn_pages_remove() to fix mmapped files changed by
  zfs rollback or zfs receive -F.
  
  PR:		kern/160035, kern/156933

Modified:
  stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
  stable/8/sys/kern/vfs_vnops.c
  stable/8/sys/sys/vnode.h
  stable/8/sys/ufs/ffs/ffs_inode.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)

Modified: stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
==============================================================================
--- stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c	Fri Sep  2 08:15:48 2011	(r225325)
+++ stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c	Fri Sep  2 08:19:19 2011	(r225326)
@@ -1259,6 +1259,7 @@ zfs_rezget(znode_t *zp)
 	zfsvfs_t *zfsvfs = zp->z_zfsvfs;
 	dmu_object_info_t doi;
 	dmu_buf_t *db;
+	vnode_t *vp;
 	uint64_t obj_num = zp->z_id;
 	uint64_t mode, size;
 	sa_bulk_attr_t bulk[8];
@@ -1334,8 +1335,9 @@ zfs_rezget(znode_t *zp)
 	 * that for example regular file was replaced with directory
 	 * which has the same object number.
 	 */
-	if (ZTOV(zp) != NULL &&
-	    ZTOV(zp)->v_type != IFTOVT((mode_t)zp->z_mode)) {
+	vp = ZTOV(zp);
+	if (vp != NULL &&
+	    vp->v_type != IFTOVT((mode_t)zp->z_mode)) {
 		zfs_znode_dmu_fini(zp);
 		ZFS_OBJ_HOLD_EXIT(zfsvfs, obj_num);
 		return (EIO);
@@ -1343,8 +1345,11 @@ zfs_rezget(znode_t *zp)
 
 	zp->z_unlinked = (zp->z_links == 0);
 	zp->z_blksz = doi.doi_data_block_size;
-	if (zp->z_size != size && ZTOV(zp) != NULL)
-		vnode_pager_setsize(ZTOV(zp), zp->z_size);
+	if (vp != NULL) {
+		vn_pages_remove(vp, 0, 0);
+		if (zp->z_size != size)
+			vnode_pager_setsize(vp, zp->z_size);
+	}
 
 	ZFS_OBJ_HOLD_EXIT(zfsvfs, obj_num);
 

Modified: stable/8/sys/kern/vfs_vnops.c
==============================================================================
--- stable/8/sys/kern/vfs_vnops.c	Fri Sep  2 08:15:48 2011	(r225325)
+++ stable/8/sys/kern/vfs_vnops.c	Fri Sep  2 08:19:19 2011	(r225326)
@@ -63,6 +63,9 @@ __FBSDID("$FreeBSD$");
 
 #include <security/mac/mac_framework.h>
 
+#include <vm/vm.h>
+#include <vm/vm_object.h>
+
 static fo_rdwr_t	vn_read;
 static fo_rdwr_t	vn_write;
 static fo_truncate_t	vn_truncate;
@@ -1353,3 +1356,15 @@ vn_rlimit_fsize(const struct vnode *vp, 
 
 	return (0);
 }
+
+void
+vn_pages_remove(struct vnode *vp, vm_pindex_t start, vm_pindex_t end)
+{
+	vm_object_t object;
+
+	if ((object = vp->v_object) == NULL)
+		return;
+	VM_OBJECT_LOCK(object);
+	vm_object_page_remove(object, start, end, 0);
+	VM_OBJECT_UNLOCK(object);
+}

Modified: stable/8/sys/sys/vnode.h
==============================================================================
--- stable/8/sys/sys/vnode.h	Fri Sep  2 08:15:48 2011	(r225325)
+++ stable/8/sys/sys/vnode.h	Fri Sep  2 08:19:19 2011	(r225326)
@@ -644,6 +644,7 @@ int	_vn_lock(struct vnode *vp, int flags
 int	vn_open(struct nameidata *ndp, int *flagp, int cmode, struct file *fp);
 int	vn_open_cred(struct nameidata *ndp, int *flagp, int cmode,
 	    u_int vn_open_flags, struct ucred *cred, struct file *fp);
+void	vn_pages_remove(struct vnode *vp, vm_pindex_t start, vm_pindex_t end);
 int	vn_pollrecord(struct vnode *vp, struct thread *p, int events);
 int	vn_rdwr(enum uio_rw rw, struct vnode *vp, void *base,
 	    int len, off_t offset, enum uio_seg segflg, int ioflg,

Modified: stable/8/sys/ufs/ffs/ffs_inode.c
==============================================================================
--- stable/8/sys/ufs/ffs/ffs_inode.c	Fri Sep  2 08:15:48 2011	(r225325)
+++ stable/8/sys/ufs/ffs/ffs_inode.c	Fri Sep  2 08:19:19 2011	(r225326)
@@ -129,18 +129,6 @@ ffs_update(vp, waitfor)
 	}
 }
 
-static void
-ffs_pages_remove(struct vnode *vp, vm_pindex_t start, vm_pindex_t end)
-{
-	vm_object_t object;
-
-	if ((object = vp->v_object) == NULL)
-		return;
-	VM_OBJECT_LOCK(object);
-	vm_object_page_remove(object, start, end, FALSE);
-	VM_OBJECT_UNLOCK(object);
-}
-
 #define	SINGLE	0	/* index of single indirect block */
 #define	DOUBLE	1	/* index of double indirect block */
 #define	TRIPLE	2	/* index of triple indirect block */
@@ -218,7 +206,7 @@ ffs_truncate(vp, length, flags, cred, td
 			(void) chkdq(ip, -extblocks, NOCRED, 0);
 #endif
 			vinvalbuf(vp, V_ALT, 0, 0);
-			ffs_pages_remove(vp,
+			vn_pages_remove(vp,
 			    OFF_TO_IDX(lblktosize(fs, -extblocks)), 0);
 			ip->i_din2->di_extsize = 0;
 			for (i = 0; i < NXADDR; i++) {
@@ -297,7 +285,7 @@ ffs_truncate(vp, length, flags, cred, td
 			ASSERT_VOP_LOCKED(vp, "ffs_truncate1");
 			vinvalbuf(vp, needextclean ? 0 : V_NORMAL, 0, 0);
 			if (!needextclean)
-				ffs_pages_remove(vp, 0,
+				vn_pages_remove(vp, 0,
 				    OFF_TO_IDX(lblktosize(fs, -extblocks)));
 			vnode_pager_setsize(vp, 0);
 			ip->i_flag |= IN_CHANGE | IN_UPDATE;
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 5 Martin Matuska freebsd_committer freebsd_triage 2011-09-02 09:23:57 UTC
State Changed
From-To: open->closed

Resolved. Thanks!