[PATCH v3 0/2] use i_lock to protect updates of i_blocks &

[PATCH v3 0/2] use i_lock to protect updates of i_blocks &

[Hou Tao]

The updates of i_blocks & i_size are buggy under 32-bit case, so
fix them accordingly.

Hou Tao (2):
  9p: use inode->i_lock to protect i_size_write() under 32-bit
  9p: use i_lock to protect update of inode->i_blocks under 32-bit

 fs/9p/v9fs_vfs.h       | 32 ++++++++++++++++++++++++++++++--
 fs/9p/vfs_file.c       |  6 +++++-
 fs/9p/vfs_inode.c      | 26 +++++++++++++-------------
 fs/9p/vfs_inode_dotl.c | 35 +++++++++++++++++++----------------
 fs/9p/vfs_super.c      |  4 ++--
 5 files changed, 69 insertions(+), 34 deletions(-)

-- 
2.16.2.dirty

[PATCH v3 1/2] 9p: use inode->i_lock to protect i_size_write() under 32-bit

[Hou Tao]

Use inode->i_lock to protect i_size_write(), else i_size_read() in
generic_fillattr() may loop infinitely in read_seqcount_begin() when
multiple processes invoke v9fs_vfs_getattr() or v9fs_vfs_getattr_dotl()
simultaneously under 32-bit SMP environment, and a soft lockup will be
triggered as show below:

  watchdog: BUG: soft lockup - CPU#5 stuck for 22s! [stat:2217]
  Modules linked in:
  CPU: 5 PID: 2217 Comm: stat Not tainted 5.0.0-rc1-00005-g7f702faf5a9e #4
  Hardware name: Generic DT based system
  PC is at generic_fillattr+0x104/0x108
  LR is at 0xec497f00
  pc : [<802b8898>]    lr : [<ec497f00>]    psr: 200c0013
  sp : ec497e20  ip : ed608030  fp : ec497e3c
  r10: 00000000  r9 : ec497f00  r8 : ed608030
  r7 : ec497ebc  r6 : ec497f00  r5 : ee5c1550  r4 : ee005780
  r3 : 0000052d  r2 : 00000000  r1 : ec497f00  r0 : ed608030
  Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
  Control: 10c5387d  Table: ac48006a  DAC: 00000051
  CPU: 5 PID: 2217 Comm: stat Not tainted 5.0.0-rc1-00005-g7f702faf5a9e #4
  Hardware name: Generic DT based system
  Backtrace:
  [<8010d974>] (dump_backtrace) from [<8010dc88>] (show_stack+0x20/0x24)
  [<8010dc68>] (show_stack) from [<80a1d194>] (dump_stack+0xb0/0xdc)
  [<80a1d0e4>] (dump_stack) from [<80109f34>] (show_regs+0x1c/0x20)
  [<80109f18>] (show_regs) from [<801d0a80>] (watchdog_timer_fn+0x280/0x2f8)
  [<801d0800>] (watchdog_timer_fn) from [<80198658>] (__hrtimer_run_queues+0x18c/0x380)
  [<801984cc>] (__hrtimer_run_queues) from [<80198e60>] (hrtimer_run_queues+0xb8/0xf0)
  [<80198da8>] (hrtimer_run_queues) from [<801973e8>] (run_local_timers+0x28/0x64)
  [<801973c0>] (run_local_timers) from [<80197460>] (update_process_times+0x3c/0x6c)
  [<80197424>] (update_process_times) from [<801ab2b8>] (tick_nohz_handler+0xe0/0x1bc)
  [<801ab1d8>] (tick_nohz_handler) from [<80843050>] (arch_timer_handler_virt+0x38/0x48)
  [<80843018>] (arch_timer_handler_virt) from [<80180a64>] (handle_percpu_devid_irq+0x8c/0x240)
  [<801809d8>] (handle_percpu_devid_irq) from [<8017ac20>] (generic_handle_irq+0x34/0x44)
  [<8017abec>] (generic_handle_irq) from [<8017b344>] (__handle_domain_irq+0x6c/0xc4)
  [<8017b2d8>] (__handle_domain_irq) from [<801022e0>] (gic_handle_irq+0x4c/0x88)
  [<80102294>] (gic_handle_irq) from [<80101a30>] (__irq_svc+0x70/0x98)
  [<802b8794>] (generic_fillattr) from [<8056b284>] (v9fs_vfs_getattr_dotl+0x74/0xa4)
  [<8056b210>] (v9fs_vfs_getattr_dotl) from [<802b8904>] (vfs_getattr_nosec+0x68/0x7c)
  [<802b889c>] (vfs_getattr_nosec) from [<802b895c>] (vfs_getattr+0x44/0x48)
  [<802b8918>] (vfs_getattr) from [<802b8a74>] (vfs_statx+0x9c/0xec)
  [<802b89d8>] (vfs_statx) from [<802b9428>] (sys_lstat64+0x48/0x78)
  [<802b93e0>] (sys_lstat64) from [<80101000>] (ret_fast_syscall+0x0/0x28)

Cc: stable@vger.kernel.org
Fixes: 7549ae3e81cc ("9p: Use the i_size_[read, write]() macros instead of using inode->i_size directly.")
Reported-by: Xing Gaopeng <xingaopeng@huawei.com>
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
v2:
	* move acquisition of i_lock into v9fs_stat2inode()
    	* rename v9fs_stat2inode() to v9fs_stat2inode_flags() to accomodate
	  v9fs_refresh_inode() which doesn't need to update i_size.
v3:
	* add a flag parameter to use v9fs_stat2inode() and use it directly
	* add helper v9fs_i_size_write() and get ride of i_lock under 64-bit
	* use v9fs_i_size_write() instead of i_size_write() in v9fs_file_write_iter
	* add Fixes and cc stable tags
---
 fs/9p/v9fs_vfs.h       | 22 ++++++++++++++++++++--
 fs/9p/vfs_file.c       |  6 +++++-
 fs/9p/vfs_inode.c      | 23 +++++++++++------------
 fs/9p/vfs_inode_dotl.c | 27 ++++++++++++++-------------
 fs/9p/vfs_super.c      |  4 ++--
 5 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
index 5a0db6dec8d1..3d371b9e461a 100644
--- a/fs/9p/v9fs_vfs.h
+++ b/fs/9p/v9fs_vfs.h
@@ -40,6 +40,9 @@
  */
 #define P9_LOCK_TIMEOUT (30*HZ)
 
+/* flags for v9fs_stat2inode() & v9fs_stat2inode_dotl() */
+#define V9FS_STAT2INODE_KEEP_ISIZE 1
+
 extern struct file_system_type v9fs_fs_type;
 extern const struct address_space_operations v9fs_addr_operations;
 extern const struct file_operations v9fs_file_operations;
@@ -61,8 +64,10 @@ int v9fs_init_inode(struct v9fs_session_info *v9ses,
 		    struct inode *inode, umode_t mode, dev_t);
 void v9fs_evict_inode(struct inode *inode);
 ino_t v9fs_qid2ino(struct p9_qid *qid);
-void v9fs_stat2inode(struct p9_wstat *, struct inode *, struct super_block *);
-void v9fs_stat2inode_dotl(struct p9_stat_dotl *, struct inode *);
+void v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
+		      struct super_block *sb, unsigned int flags);
+void v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode,
+			   unsigned int flags);
 int v9fs_dir_release(struct inode *inode, struct file *filp);
 int v9fs_file_open(struct inode *inode, struct file *file);
 void v9fs_inode2stat(struct inode *inode, struct p9_wstat *stat);
@@ -83,4 +88,17 @@ static inline void v9fs_invalidate_inode_attr(struct inode *inode)
 }
 
 int v9fs_open_to_dotl_flags(int flags);
+
+static inline void v9fs_i_size_write(struct inode *inode, loff_t i_size)
+{
+	/*
+	 * Avoid taking i_lock under 64-bit.
+	 * Refer to fsstack_copy_inode_size() for detailed explanation.
+	 */
+	if (sizeof(i_size) > sizeof(long))
+		spin_lock(&inode->i_lock);
+	i_size_write(inode, i_size);
+	if (sizeof(i_size) > sizeof(long))
+		spin_unlock(&inode->i_lock);
+}
 #endif
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index a25efa782fcc..9a1125305d84 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -446,7 +446,11 @@ v9fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		i_size = i_size_read(inode);
 		if (iocb->ki_pos > i_size) {
 			inode_add_bytes(inode, iocb->ki_pos - i_size);
-			i_size_write(inode, iocb->ki_pos);
+			/*
+			 * Need to serialize against i_size_write() in
+			 * v9fs_stat2inode()
+			 */
+			v9fs_i_size_write(inode, iocb->ki_pos);
 		}
 		return retval;
 	}
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 85ff859d3af5..72b779bc0942 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -538,7 +538,7 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
 	if (retval)
 		goto error;
 
-	v9fs_stat2inode(st, inode, sb);
+	v9fs_stat2inode(st, inode, sb, 0);
 	v9fs_cache_inode_get_cookie(inode);
 	unlock_new_inode(inode);
 	return inode;
@@ -1092,7 +1092,7 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat,
 	if (IS_ERR(st))
 		return PTR_ERR(st);
 
-	v9fs_stat2inode(st, d_inode(dentry), dentry->d_sb);
+	v9fs_stat2inode(st, d_inode(dentry), dentry->d_sb, 0);
 	generic_fillattr(d_inode(dentry), stat);
 
 	p9stat_free(st);
@@ -1170,12 +1170,13 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
  * @stat: Plan 9 metadata (mistat) structure
  * @inode: inode to populate
  * @sb: superblock of filesystem
+ * @flags: control flags (e.g. V9FS_STAT2INODE_KEEP_ISIZE)
  *
  */
 
 void
 v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
-	struct super_block *sb)
+		 struct super_block *sb, unsigned int flags)
 {
 	umode_t mode;
 	char ext[32];
@@ -1216,10 +1217,11 @@ v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
 	mode = p9mode2perm(v9ses, stat);
 	mode |= inode->i_mode & ~S_IALLUGO;
 	inode->i_mode = mode;
-	i_size_write(inode, stat->length);
 
+	if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE))
+		v9fs_i_size_write(inode, stat->length);
 	/* not real number of blocks, but 512 byte ones ... */
-	inode->i_blocks = (i_size_read(inode) + 512 - 1) >> 9;
+	inode->i_blocks = (stat->length + 512 - 1) >> 9;
 	v9inode->cache_validity &= ~V9FS_INO_INVALID_ATTR;
 }
 
@@ -1416,9 +1418,9 @@ int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
 {
 	int umode;
 	dev_t rdev;
-	loff_t i_size;
 	struct p9_wstat *st;
 	struct v9fs_session_info *v9ses;
+	unsigned int flags;
 
 	v9ses = v9fs_inode2v9ses(inode);
 	st = p9_client_stat(fid);
@@ -1431,16 +1433,13 @@ int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
 	if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
 		goto out;
 
-	spin_lock(&inode->i_lock);
 	/*
 	 * We don't want to refresh inode->i_size,
 	 * because we may have cached data
 	 */
-	i_size = inode->i_size;
-	v9fs_stat2inode(st, inode, inode->i_sb);
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		inode->i_size = i_size;
-	spin_unlock(&inode->i_lock);
+	flags = (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) ?
+		V9FS_STAT2INODE_KEEP_ISIZE : 0;
+	v9fs_stat2inode(st, inode, inode->i_sb, flags);
 out:
 	p9stat_free(st);
 	kfree(st);
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 4823e1c46999..a950a927a626 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -143,7 +143,7 @@ static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
 	if (retval)
 		goto error;
 
-	v9fs_stat2inode_dotl(st, inode);
+	v9fs_stat2inode_dotl(st, inode, 0);
 	v9fs_cache_inode_get_cookie(inode);
 	retval = v9fs_get_acl(inode, fid);
 	if (retval)
@@ -496,7 +496,7 @@ v9fs_vfs_getattr_dotl(const struct path *path, struct kstat *stat,
 	if (IS_ERR(st))
 		return PTR_ERR(st);
 
-	v9fs_stat2inode_dotl(st, d_inode(dentry));
+	v9fs_stat2inode_dotl(st, d_inode(dentry), 0);
 	generic_fillattr(d_inode(dentry), stat);
 	/* Change block size to what the server returned */
 	stat->blksize = st->st_blksize;
@@ -607,11 +607,13 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
  * v9fs_stat2inode_dotl - populate an inode structure with stat info
  * @stat: stat structure
  * @inode: inode to populate
+ * @flags: ctrl flags (e.g. V9FS_STAT2INODE_KEEP_ISIZE)
  *
  */
 
 void
-v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode)
+v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode,
+		      unsigned int flags)
 {
 	umode_t mode;
 	struct v9fs_inode *v9inode = V9FS_I(inode);
@@ -631,7 +633,8 @@ v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode)
 		mode |= inode->i_mode & ~S_IALLUGO;
 		inode->i_mode = mode;
 
-		i_size_write(inode, stat->st_size);
+		if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE))
+			v9fs_i_size_write(inode, stat->st_size);
 		inode->i_blocks = stat->st_blocks;
 	} else {
 		if (stat->st_result_mask & P9_STATS_ATIME) {
@@ -661,8 +664,9 @@ v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode)
 		}
 		if (stat->st_result_mask & P9_STATS_RDEV)
 			inode->i_rdev = new_decode_dev(stat->st_rdev);
-		if (stat->st_result_mask & P9_STATS_SIZE)
-			i_size_write(inode, stat->st_size);
+		if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE) &&
+		    stat->st_result_mask & P9_STATS_SIZE)
+			v9fs_i_size_write(inode, stat->st_size);
 		if (stat->st_result_mask & P9_STATS_BLOCKS)
 			inode->i_blocks = stat->st_blocks;
 	}
@@ -928,9 +932,9 @@ v9fs_vfs_get_link_dotl(struct dentry *dentry,
 
 int v9fs_refresh_inode_dotl(struct p9_fid *fid, struct inode *inode)
 {
-	loff_t i_size;
 	struct p9_stat_dotl *st;
 	struct v9fs_session_info *v9ses;
+	unsigned int flags;
 
 	v9ses = v9fs_inode2v9ses(inode);
 	st = p9_client_getattr_dotl(fid, P9_STATS_ALL);
@@ -942,16 +946,13 @@ int v9fs_refresh_inode_dotl(struct p9_fid *fid, struct inode *inode)
 	if ((inode->i_mode & S_IFMT) != (st->st_mode & S_IFMT))
 		goto out;
 
-	spin_lock(&inode->i_lock);
 	/*
 	 * We don't want to refresh inode->i_size,
 	 * because we may have cached data
 	 */
-	i_size = inode->i_size;
-	v9fs_stat2inode_dotl(st, inode);
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		inode->i_size = i_size;
-	spin_unlock(&inode->i_lock);
+	flags = (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) ?
+		V9FS_STAT2INODE_KEEP_ISIZE : 0;
+	v9fs_stat2inode_dotl(st, inode, flags);
 out:
 	kfree(st);
 	return 0;
diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index 48ce50484e80..eeab9953af89 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -172,7 +172,7 @@ static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags,
 			goto release_sb;
 		}
 		d_inode(root)->i_ino = v9fs_qid2ino(&st->qid);
-		v9fs_stat2inode_dotl(st, d_inode(root));
+		v9fs_stat2inode_dotl(st, d_inode(root), 0);
 		kfree(st);
 	} else {
 		struct p9_wstat *st = NULL;
@@ -183,7 +183,7 @@ static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags,
 		}
 
 		d_inode(root)->i_ino = v9fs_qid2ino(&st->qid);
-		v9fs_stat2inode(st, d_inode(root), sb);
+		v9fs_stat2inode(st, d_inode(root), sb, 0);
 
 		p9stat_free(st);
 		kfree(st);
-- 
2.16.2.dirty

[PATCH v3 2/2] 9p: use i_lock to protect update of inode->i_blocks under 32-bit

[Hou Tao]

When v9fs_stat2inode() is invoked concurrently under 32-bit case
and CONFIG_LBDAF is enabled, multiple updates of inode->i_blocks
may corrupt the value of i_blocks because the assignment of 64-bit
value under 32-bit host is not atomic.

Fix it by using i_lock to protect update of inode->i_blocks. Also
Skip the update when it's not requested.

Suggested-by: Dominique Martinet <asmadeus@codewreck.org>
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/9p/v9fs_vfs.h       | 10 ++++++++++
 fs/9p/vfs_inode.c      |  7 ++++---
 fs/9p/vfs_inode_dotl.c | 16 +++++++++-------
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
index 3d371b9e461a..0bd4acfdb6af 100644
--- a/fs/9p/v9fs_vfs.h
+++ b/fs/9p/v9fs_vfs.h
@@ -101,4 +101,14 @@ static inline void v9fs_i_size_write(struct inode *inode, loff_t i_size)
 	if (sizeof(i_size) > sizeof(long))
 		spin_unlock(&inode->i_lock);
 }
+
+static inline void v9fs_i_blocks_write(struct inode *inode, blkcnt_t i_blocks)
+{
+	/* Avoid taking i_lock under 64-bits */
+	if (sizeof(i_blocks) > sizeof(long))
+		spin_lock(&inode->i_lock);
+	inode->i_blocks = i_blocks;
+	if (sizeof(i_blocks) > sizeof(long))
+		spin_unlock(&inode->i_lock);
+}
 #endif
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 72b779bc0942..f05ff3cfbfe7 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1218,10 +1218,11 @@ v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
 	mode |= inode->i_mode & ~S_IALLUGO;
 	inode->i_mode = mode;
 
-	if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE))
+	if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE)) {
 		v9fs_i_size_write(inode, stat->length);
-	/* not real number of blocks, but 512 byte ones ... */
-	inode->i_blocks = (stat->length + 512 - 1) >> 9;
+		/* not real number of blocks, but 512 byte ones ... */
+		v9fs_i_blocks_write(inode, (stat->length + 512 - 1) >> 9);
+	}
 	v9inode->cache_validity &= ~V9FS_INO_INVALID_ATTR;
 }
 
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index a950a927a626..c1e2416d83c1 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -633,9 +633,10 @@ v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode,
 		mode |= inode->i_mode & ~S_IALLUGO;
 		inode->i_mode = mode;
 
-		if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE))
+		if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE)) {
 			v9fs_i_size_write(inode, stat->st_size);
-		inode->i_blocks = stat->st_blocks;
+			v9fs_i_blocks_write(inode, stat->st_blocks);
+		}
 	} else {
 		if (stat->st_result_mask & P9_STATS_ATIME) {
 			inode->i_atime.tv_sec = stat->st_atime_sec;
@@ -664,11 +665,12 @@ v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode,
 		}
 		if (stat->st_result_mask & P9_STATS_RDEV)
 			inode->i_rdev = new_decode_dev(stat->st_rdev);
-		if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE) &&
-		    stat->st_result_mask & P9_STATS_SIZE)
-			v9fs_i_size_write(inode, stat->st_size);
-		if (stat->st_result_mask & P9_STATS_BLOCKS)
-			inode->i_blocks = stat->st_blocks;
+		if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE)) {
+			if (stat->st_result_mask & P9_STATS_SIZE)
+				v9fs_i_size_write(inode, stat->st_size);
+			if (stat->st_result_mask & P9_STATS_BLOCKS)
+				v9fs_i_blocks_write(inode, stat->st_blocks);
+		}
 	}
 	if (stat->st_result_mask & P9_STATS_GEN)
 		inode->i_generation = stat->st_gen;
-- 
2.16.2.dirty

Re: [PATCH v3 1/2] 9p: use inode->i_lock to protect i_size_write() under 32-bit

[Dominique Martinet]

Hou Tao wrote on Thu, Jan 24, 2019:
> @@ -83,4 +88,17 @@ static inline void v9fs_invalidate_inode_attr(struct inode *inode)
>  }
>  
>  int v9fs_open_to_dotl_flags(int flags);
> +
> +static inline void v9fs_i_size_write(struct inode *inode, loff_t i_size)
> +{
> +	/*
> +	 * Avoid taking i_lock under 64-bit.
> +	 * Refer to fsstack_copy_inode_size() for detailed explanation.
> +	 */

Just one last nitpick here: I'd rather not reference to another function
for comment; the 9p code doesn't change much and if someone ever changes
fsstack_copy_inode_size it'll probably go unnoticed for a while.

If you're OK with this I can change it myself, but got other comments
for second patch so up to you.

More than happy with the rest, I'll run some tests over the weekend for
the sake of it but should take this patch to -next early next week
assuming we agree on something for the comment.

-- 
Dominique

Re: [PATCH v3 2/2] 9p: use i_lock to protect update of inode->i_blocks under 32-bit

[Dominique Martinet]

Hou Tao wrote on Thu, Jan 24, 2019:
> When v9fs_stat2inode() is invoked concurrently under 32-bit case
> and CONFIG_LBDAF is enabled, multiple updates of inode->i_blocks
> may corrupt the value of i_blocks because the assignment of 64-bit
> value under 32-bit host is not atomic.
> 
> Fix it by using i_lock to protect update of inode->i_blocks. Also
> Skip the update when it's not requested.
> 
> Suggested-by: Dominique Martinet <asmadeus@codewreck.org>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  fs/9p/v9fs_vfs.h       | 10 ++++++++++
>  fs/9p/vfs_inode.c      |  7 ++++---
>  fs/9p/vfs_inode_dotl.c | 16 +++++++++-------
>  3 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
> index 3d371b9e461a..0bd4acfdb6af 100644
> --- a/fs/9p/v9fs_vfs.h
> +++ b/fs/9p/v9fs_vfs.h
> @@ -101,4 +101,14 @@ static inline void v9fs_i_size_write(struct inode *inode, loff_t i_size)
>  	if (sizeof(i_size) > sizeof(long))
>  		spin_unlock(&inode->i_lock);
>  }
> +
> +static inline void v9fs_i_blocks_write(struct inode *inode, blkcnt_t i_blocks)
> +{
> +	/* Avoid taking i_lock under 64-bits */
> +	if (sizeof(i_blocks) > sizeof(long))
> +		spin_lock(&inode->i_lock);
> +	inode->i_blocks = i_blocks;
> +	if (sizeof(i_blocks) > sizeof(long))
> +		spin_unlock(&inode->i_lock);
> +}
>  #endif
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 72b779bc0942..f05ff3cfbfe7 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -1218,10 +1218,11 @@ v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
>  	mode |= inode->i_mode & ~S_IALLUGO;
>  	inode->i_mode = mode;
>  
> -	if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE))
> +	if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE)) {
>  		v9fs_i_size_write(inode, stat->length);
> -	/* not real number of blocks, but 512 byte ones ... */
> -	inode->i_blocks = (stat->length + 512 - 1) >> 9;
> +		/* not real number of blocks, but 512 byte ones ... */
> +		v9fs_i_blocks_write(inode, (stat->length + 512 - 1) >> 9);

hmm, part of me wants to abuse v9fs_i_size_write to also update i_blocks
as I think we'd usually want to keep both in sync.

In v9fs_file_write_iter we do not update i_blocks directly but
inode_add_bytes basically does the same thing... Speaking of which it's
assuming inode->i_bytes is set correctly initially; I _think_ we're
supposed to call inode_set_bytes() instead of writing i_blocks directly
in stat2inode?
Sorry, I'm not too familiar with this code and there doesn't seem to be
many users of inode_set_bytes so it's a bit puzzling to me.

I'm not using cache mode much but I have some very weird behaviour
currently if I mount something with cache=fscache :
 - if I create a new file from 9p and stat on the same client, a size of
 0-511 has block = 0 then it's just always off by 1
 - if I create a file outside of 9p and stat it, for example starting
 with 400 bytes, it starts with 8 blocks then the number of blocks
 increase by 1 everytime 512 bytes are written regardless of initial
 size.

Let's try to go back to some decent behaviour there first, and then
we can decide if it still make sense to separate v9fs_i_blocks_write and
v9fs_i_size_write or not.

-- 
Dominique

Re: [PATCH v3 1/2] 9p: use inode->i_lock to protect i_size_write() under 32-bit

[Hou Tao]

Hi,

On 2019/1/24 14:57, Dominique Martinet wrote:
> Hou Tao wrote on Thu, Jan 24, 2019:
>> @@ -83,4 +88,17 @@ static inline void v9fs_invalidate_inode_attr(struct inode *inode)
>>  }
>>  
>>  int v9fs_open_to_dotl_flags(int flags);
>> +
>> +static inline void v9fs_i_size_write(struct inode *inode, loff_t i_size)
>> +{
>> +	/*
>> +	 * Avoid taking i_lock under 64-bit.
>> +	 * Refer to fsstack_copy_inode_size() for detailed explanation.
>> +	 */
> 
> Just one last nitpick here: I'd rather not reference to another function
> for comment; the 9p code doesn't change much and if someone ever changes
> fsstack_copy_inode_size it'll probably go unnoticed for a while.
> 
> If you're OK with this I can change it myself, but got other comments
> for second patch so up to you.
Please help to change it, thanks. I will update the second patch accordingly
or let it referring to v9fs_i_size_write :)

Regards,
Tao
> 
> More than happy with the rest, I'll run some tests over the weekend for
> the sake of it but should take this patch to -next early next week
> assuming we agree on something for the comment.
> 

Re: [PATCH v3 2/2] 9p: use i_lock to protect update of inode->i_blocks under 32-bit

[Hou Tao]

Hi,

On 2019/1/24 15:25, Dominique Martinet wrote:
> Hou Tao wrote on Thu, Jan 24, 2019:
>> When v9fs_stat2inode() is invoked concurrently under 32-bit case
>> and CONFIG_LBDAF is enabled, multiple updates of inode->i_blocks
>> may corrupt the value of i_blocks because the assignment of 64-bit
>> value under 32-bit host is not atomic.
>>
>> Fix it by using i_lock to protect update of inode->i_blocks. Also
>> Skip the update when it's not requested.
>>
>> Suggested-by: Dominique Martinet <asmadeus@codewreck.org>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  fs/9p/v9fs_vfs.h       | 10 ++++++++++
>>  fs/9p/vfs_inode.c      |  7 ++++---
>>  fs/9p/vfs_inode_dotl.c | 16 +++++++++-------
>>  3 files changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
>> index 3d371b9e461a..0bd4acfdb6af 100644
>> --- a/fs/9p/v9fs_vfs.h
>> +++ b/fs/9p/v9fs_vfs.h
>> @@ -101,4 +101,14 @@ static inline void v9fs_i_size_write(struct inode *inode, loff_t i_size)
>>  	if (sizeof(i_size) > sizeof(long))
>>  		spin_unlock(&inode->i_lock);
>>  }
>> +
>> +static inline void v9fs_i_blocks_write(struct inode *inode, blkcnt_t i_blocks)
>> +{
>> +	/* Avoid taking i_lock under 64-bits */
>> +	if (sizeof(i_blocks) > sizeof(long))
>> +		spin_lock(&inode->i_lock);
>> +	inode->i_blocks = i_blocks;
>> +	if (sizeof(i_blocks) > sizeof(long))
>> +		spin_unlock(&inode->i_lock);
>> +}
>>  #endif
>> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
>> index 72b779bc0942..f05ff3cfbfe7 100644
>> --- a/fs/9p/vfs_inode.c
>> +++ b/fs/9p/vfs_inode.c
>> @@ -1218,10 +1218,11 @@ v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
>>  	mode |= inode->i_mode & ~S_IALLUGO;
>>  	inode->i_mode = mode;
>>  
>> -	if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE))
>> +	if (!(flags & V9FS_STAT2INODE_KEEP_ISIZE)) {
>>  		v9fs_i_size_write(inode, stat->length);
>> -	/* not real number of blocks, but 512 byte ones ... */
>> -	inode->i_blocks = (stat->length + 512 - 1) >> 9;
>> +		/* not real number of blocks, but 512 byte ones ... */
>> +		v9fs_i_blocks_write(inode, (stat->length + 512 - 1) >> 9);
> 
> hmm, part of me wants to abuse v9fs_i_size_write to also update i_blocks
> as I think we'd usually want to keep both in sync.We can do that for v9fs_stat2inode(), but for v9fs_stat2inode_dotl()
the updates of i_block & i_size are controlled by different flags, so
separated updates are needed (maybe also add an extra flags ?).

> 
> In v9fs_file_write_iter we do not update i_blocks directly but
> inode_add_bytes basically does the same thing... Speaking of which it's
> assuming inode->i_bytes is set correctly initially; I _think_ we're
> supposed to call inode_set_bytes() instead of writing i_blocks directly
> in stat2inode?
> Sorry, I'm not too familiar with this code and there doesn't seem to be
> many users of inode_set_bytes so it's a bit puzzling to me.
> 
In my understanding, now only disk quota uses i_bytes, so for 9p there is not so
much different between updating i_blocks directly and using inode_set_bytes(),
but maybe using inode_set_bytes() will be appropriate.


> I'm not using cache mode much but I have some very weird behaviour
> currently if I mount something with cache=fscache :
>  - if I create a new file from 9p and stat on the same client, a size of
>  0-511 has block = 0 then it's just always off by 1
Is this not the expected result ? v9fs_write_end() will call inode_add_bytes(inode,511)
if we write 511 bytes to the new file and i_blocks will be 0.

>  - if I create a file outside of 9p and stat it, for example starting
>  with 400 bytes, it starts with 8 blocks then the number of blocks
>  increase by 1 everytime 512 bytes are written regardless of initial
>  size.
> 
The initial i_blocks comes from the underlying filesystem, so I think 8 blocks
just mean the block size of the underlying filesystem is 4KB. If another 512 bytes
are written into the file, inode_add_bytes() will increase i_blocks by one instead of
fetching i_blocks from server, though i_blocks in server side will still be 8. Maybe
we should call v9fs_invalidate_inode_attr() in v9fs_write_end() ?

> Let's try to go back to some decent behaviour there first, and then
> we can decide if it still make sense to separate v9fs_i_blocks_write and
> v9fs_i_size_write or not.
> 
I don't follow that. Could you elaborate ?

Regards,
Tao

Re: [PATCH v3 2/2] 9p: use i_lock to protect update of inode->i_blocks under 32-bit

[Dominique Martinet]

Hou Tao wrote on Thu, Jan 24, 2019:
> > hmm, part of me wants to abuse v9fs_i_size_write to also update i_blocks
> > as I think we'd usually want to keep both in sync.We can do that for
> > v9fs_stat2inode(), but for v9fs_stat2inode_dotl()
>
> the updates of i_block & i_size are controlled by different flags, so
> separated updates are needed (maybe also add an extra flags ?).

Is there any case where i_block and i_size are not updated together?
I would be happy with an extra flag if required, I just do not see where
right now (admitedly didn't look for long, will have a second look
tomorrow)


> > In v9fs_file_write_iter we do not update i_blocks directly but
> > inode_add_bytes basically does the same thing... Speaking of which it's
> > assuming inode->i_bytes is set correctly initially; I _think_ we're
> > supposed to call inode_set_bytes() instead of writing i_blocks directly
> > in stat2inode?
> > Sorry, I'm not too familiar with this code and there doesn't seem to be
> > many users of inode_set_bytes so it's a bit puzzling to me.
>
> In my understanding, now only disk quota uses i_bytes, so for 9p there is not so
> much different between updating i_blocks directly and using inode_set_bytes(),
> but maybe using inode_set_bytes() will be appropriate.

I also think only quota uses i_bytes directly, but 9p also uses it
indirectly through inode_add_bytes() - i_blocks is incremented by 1
every 512 bytes and i_bytes is reduced to its last few bits (basically
%512)


> > I'm not using cache mode much but I have some very weird behaviour
> > currently if I mount something with cache=fscache :
> >  - if I create a new file from 9p and stat on the same client, a size of
> >  0-511 has block = 0 then it's just always off by 1

> Is this not the expected result ? v9fs_write_end() will call
> inode_add_bytes(inode,511) if we write 511 bytes to the new file and
> i_blocks will be 0.

That is the obvious result from the current code, yes - but it
definitely is wrong.
i_blocks should be 1 from byte 1 onwards until 512, and bumped to 2 at
513. From a filesystem point of view, a block is already "consumed" from
the first byte onward.
(This isn't what inode_set_bytes does..)


In practice, this actually is fairly important - tools like tar are
optimized such that if a file has no blocks it is considered sparse and
isn't read at all, so this could lead to data loss.
Interestingly I cannot reproduce with a recent gnu tar but I am sure I
have seen the behaviour (some lustre bug[1] talks about this as well and
the code is still present[2]); we should not report a st_blocks of 0 is
there is data -- v9fs_stat2inode (not v9fs_stat2inode_dotl) also rounds
up i_blocks so this is the right behaviour, we're just not doing it when
creating new files apparently.

[1] https://jira.whamcloud.com/browse/LU-3864
[2] http://git.savannah.gnu.org/cgit/tar.git/tree/src/sparse.c#n273


If this optimization didn't exist and given i_bytes existence it does
seem more correct to do like inode_set_bytes (e.g. blocks = bytes / 512
and bytes = bytes % 512) but this really strikes me as odd; this might
be why hardly anyone uses this...


> >  - if I create a file outside of 9p and stat it, for example starting
> >  with 400 bytes, it starts with 8 blocks then the number of blocks
> >  increase by 1 everytime 512 bytes are written regardless of initial
> >  size.
>
> The initial i_blocks comes from the underlying filesystem, so I think 8 blocks
> just mean the block size of the underlying filesystem is 4KB. If another 512 bytes
> are written into the file, inode_add_bytes() will increase i_blocks by one instead of
> fetching i_blocks from server, though i_blocks in server side will
> still be 8.

Ah, I had only looked at v9fs_stat2inode() which creates the value from
i_size, not v9fs_stat2inode_dotl().
v9fs_stat2inode_dotl() does take the value from the RPC, and then 8 is
indeed correct -- it will just be very painful to keep synchronized with
the underlying fs; we have no ways of knowing when the remote filesystem
decides to allocate new blocks.
If for example the write iter only writes 0 and the underlying fs is
optimized st_blocks might not increase at all.

> Maybe we should call v9fs_invalidate_inode_attr() in v9fs_write_end() ?

Hmm, That will probably add quite a lot of RPCs and make the cache
option rather useless...
Honestly as far as I'm concerned the value of i_blocks is mostly
informative (it's used by tools like du to give a size estimate, and
that's about it?) - with the exception of the 0 value I was talking
earlier.

I'd advocate to never display the remote st_blocks but do like
v9fs_stat2inode() and always just set blocks appropriately from the
size (i.e. i_blocks = (i_size + 512 - 1) >> 9 and i_bytes = 0), but I'd
like some opinions first.
I'll send a mail to fsdevel asking vfs folks for advices tomorrow; let's
put this patch on hold for a few days.


Thanks,
-- 
Dominique

Re: [PATCH v3 2/2] 9p: use i_lock to protect update of inode->i_blocks under 32-bit

[Dominique Martinet]

Dominique Martinet wrote on Thu, Jan 24, 2019:
> > >  - if I create a file outside of 9p and stat it, for example starting
> > >  with 400 bytes, it starts with 8 blocks then the number of blocks
> > >  increase by 1 everytime 512 bytes are written regardless of initial
> > >  size.
> >
> > The initial i_blocks comes from the underlying filesystem, so I think 8 blocks
> > just mean the block size of the underlying filesystem is 4KB. If another 512 bytes
> > are written into the file, inode_add_bytes() will increase i_blocks by one instead of
> > fetching i_blocks from server, though i_blocks in server side will
> > still be 8.
> 
> Ah, I had only looked at v9fs_stat2inode() which creates the value from
> i_size, not v9fs_stat2inode_dotl().
> v9fs_stat2inode_dotl() does take the value from the RPC, and then 8 is
> indeed correct -- it will just be very painful to keep synchronized with
> the underlying fs; we have no ways of knowing when the remote filesystem
> decides to allocate new blocks.
> If for example the write iter only writes 0 and the underlying fs is
> optimized st_blocks might not increase at all.
> 
> > Maybe we should call v9fs_invalidate_inode_attr() in v9fs_write_end() ?
> 
> Hmm, That will probably add quite a lot of RPCs and make the cache
> option rather useless...

Actually I made an interesting point without realizing it myself -
thinking back of the option, I would keep the current behaviour with
cache=none/mmap (as the inode attr will be invalid and refreshed on
every stat anyway - this incidentally probably is why I never noticed
it, cache=none works just fine as far as st_blocks is concerned because
it always returns the underlying storage's block count) and just 100%
fake it for cache=fscache/loose.

Still going to bring this whole subject up in a separate thread to
fsdevel tomorrow, I'll put you and the v9fs-developer list in cc.


Will also add a note to run more testings with various cache
options...

-- 
Dominique

Re: [PATCH v3 1/2] 9p: use inode->i_lock to protect i_size_write() under 32-bit

[Hou Tao]

Hi Dominique,

I can not find the patch in your tree [1], so will the patch be landed in v5.1 ?
Or are you expecting a v3 patch ?

Regards,
Hou

[1]: git://github.com/martinetd/linux

On 2019/1/24 14:57, Dominique Martinet wrote:
> Hou Tao wrote on Thu, Jan 24, 2019:
>> @@ -83,4 +88,17 @@ static inline void v9fs_invalidate_inode_attr(struct inode *inode)
>>  }
>>  
>>  int v9fs_open_to_dotl_flags(int flags);
>> +
>> +static inline void v9fs_i_size_write(struct inode *inode, loff_t i_size)
>> +{
>> +	/*
>> +	 * Avoid taking i_lock under 64-bit.
>> +	 * Refer to fsstack_copy_inode_size() for detailed explanation.
>> +	 */
> 
> Just one last nitpick here: I'd rather not reference to another function
> for comment; the 9p code doesn't change much and if someone ever changes
> fsstack_copy_inode_size it'll probably go unnoticed for a while.
> 
> If you're OK with this I can change it myself, but got other comments
> for second patch so up to you.
> 
> More than happy with the rest, I'll run some tests over the weekend for
> the sake of it but should take this patch to -next early next week
> assuming we agree on something for the comment.
> 

Re: [PATCH v3 1/2] 9p: use inode->i_lock to protect i_size_write() under 32-bit

[Dominique Martinet]

Hou Tao wrote on Sat, Mar 02, 2019:
> I can not find the patch in your tree [1], so will the patch be landed in v5.1 ?
> Or are you expecting a v3 patch ?

Hmm right, sorry - I just didn't have much time for this and hoped I
could find time to sort the discussion we had on the second patch to
take both at once, but this definitely won't happen for 5.1 so I should
take the first patch for now. Will do that today after basic tests have
finished.
I should have taken it into -next earlier in principle but it can still
make it in 5.1, thanks for the reminder.


As for i_blocks, after some more thought and discussing off-list with
Jeff (Layton), I'm more or less decided that the best would be to make
this up straight from the size whenever size is updated.
"blocks" on a network FS does not really make sense to me but I
definitely want to avoid the case we have with i_blocks being 0
immediately after writing to a new file.
After your first patch I think the simplest thing to do would be to just
update i_blocks together with i_size in v9fs_i_size_write().

That's not a new bug however so I don't want to rush this in for 5.1
this late in the cycle so will take a more conservative approach on this
one, feel free to send a patch otherwise I will do once the next cycle
starts.

-- 
Dominique