diff mbox series

[11/11] afs: Fix dirty-region encoding on ppc32 with 64K pages

Message ID 160389426655.300137.17487677797144804730.stgit@warthog.procyon.org.uk (mailing list archive)
State New, archived
Headers show
Series AFS fixes [ver #2] | expand

Commit Message

David Howells Oct. 28, 2020, 2:11 p.m. UTC
The dirty region bounds stored in page->private on an afs page are 15 bits
on a 32-bit box and can, at most, represent a range of up to 32K within a
32K page with a resolution of 1 byte.  This is a problem for powerpc32 with
64K pages enabled.

Further, transparent huge pages may get up to 2M, which will be a problem
for the afs filesystem on all 32-bit arches in the future.

Fix this by decreasing the resolution.  For the moment, a 64K page will
have a resolution determined from PAGE_SIZE.  In the future, the page will
need to be passed in to the helper functions so that the page size can be
assessed and the resolution determined dynamically.

Note that this might not be the ideal way to handle this, since it may
allow some leakage of undirtied zero bytes to the server's copy in the case
of a 3rd-party conflict.  Fixing that would require a separately allocated
record and is a more complicated fix.

Fixes: 4343d00872e1 ("afs: Get rid of the afs_writeback record")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Matthew Wilcox (Oracle) <willy@infradead.org>
---

 fs/afs/internal.h |   24 +++++++++++++++++++++---
 fs/afs/write.c    |    5 -----
 2 files changed, 21 insertions(+), 8 deletions(-)

Comments

Matthew Wilcox Oct. 28, 2020, 2:34 p.m. UTC | #1
On Wed, Oct 28, 2020 at 02:11:06PM +0000, David Howells wrote:
> +static inline unsigned int afs_page_dirty_resolution(void)

I've been using size_t for offsets within a struct page.  I don't know
that we'll ever support pages larger than 2GB (they're completely
impractical with today's bus speeds), but I'd rather not be the one
who has to track down all the uses of 'int' in the kernel in fifteen
years time.

> +{
> +	if (PAGE_SIZE - 1 <= __AFS_PAGE_PRIV_MASK)
> +		return 1;
> +	else
> +		return PAGE_SIZE / (__AFS_PAGE_PRIV_MASK + 1);

Could this be DIV_ROUND_UP(PAGE_SIZE, __AFS_PAGE_PRIV_MASK + 1); avoiding
a conditional?  I appreciate it's calculated at compile time today, but
it'll be dynamic with THP.

>  static inline unsigned int afs_page_dirty_to(unsigned long priv)
>  {
> -	return ((priv >> __AFS_PAGE_PRIV_SHIFT) & __AFS_PAGE_PRIV_MASK) + 1;
> +	unsigned int x = (priv >> __AFS_PAGE_PRIV_SHIFT) & __AFS_PAGE_PRIV_MASK;
> +
> +	/* The upper bound is exclusive */

I think you mean 'inclusive'.

> +	return (x + 1) * afs_page_dirty_resolution();
>  }
>  
>  static inline unsigned long afs_page_dirty(unsigned int from, unsigned int to)
>  {
> +	unsigned int res = afs_page_dirty_resolution();
> +	from /= res; /* Round down */
> +	to = (to + res - 1) / res; /* Round up */
>  	return ((unsigned long)(to - 1) << __AFS_PAGE_PRIV_SHIFT) | from;

Wouldn't it produce the same result to just round down?  ie:

	to = (to - 1) / res;
	return ((unsigned long)to << __AFS_PAGE_PRIV_SHIFT) | from;
diff mbox series

Patch

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 214b8a239a79..52f77204f092 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -861,7 +861,8 @@  struct afs_vnode_cache_aux {
 /*
  * We use page->private to hold the amount of the page that we've written to,
  * splitting the field into two parts.  However, we need to represent a range
- * 0...PAGE_SIZE inclusive, so we can't support 64K pages on a 32-bit system.
+ * 0...PAGE_SIZE, so we reduce the resolution if the size of the page
+ * exceeds what we can encode.
  */
 #ifdef CONFIG_64BIT
 #define __AFS_PAGE_PRIV_MASK	0x7fffffffUL
@@ -873,18 +874,35 @@  struct afs_vnode_cache_aux {
 #define __AFS_PAGE_PRIV_MMAPPED	0x8000UL
 #endif
 
+static inline unsigned int afs_page_dirty_resolution(void)
+{
+	if (PAGE_SIZE - 1 <= __AFS_PAGE_PRIV_MASK)
+		return 1;
+	else
+		return PAGE_SIZE / (__AFS_PAGE_PRIV_MASK + 1);
+}
+
 static inline unsigned int afs_page_dirty_from(unsigned long priv)
 {
-	return priv & __AFS_PAGE_PRIV_MASK;
+	unsigned int x = priv & __AFS_PAGE_PRIV_MASK;
+
+	/* The lower bound is inclusive */
+	return x * afs_page_dirty_resolution();
 }
 
 static inline unsigned int afs_page_dirty_to(unsigned long priv)
 {
-	return ((priv >> __AFS_PAGE_PRIV_SHIFT) & __AFS_PAGE_PRIV_MASK) + 1;
+	unsigned int x = (priv >> __AFS_PAGE_PRIV_SHIFT) & __AFS_PAGE_PRIV_MASK;
+
+	/* The upper bound is exclusive */
+	return (x + 1) * afs_page_dirty_resolution();
 }
 
 static inline unsigned long afs_page_dirty(unsigned int from, unsigned int to)
 {
+	unsigned int res = afs_page_dirty_resolution();
+	from /= res; /* Round down */
+	to = (to + res - 1) / res; /* Round up */
 	return ((unsigned long)(to - 1) << __AFS_PAGE_PRIV_SHIFT) | from;
 }
 
diff --git a/fs/afs/write.c b/fs/afs/write.c
index e7d6827024bf..4578b372514f 100644
--- a/fs/afs/write.c
+++ b/fs/afs/write.c
@@ -90,11 +90,6 @@  int afs_write_begin(struct file *file, struct address_space *mapping,
 	_enter("{%llx:%llu},{%lx},%u,%u",
 	       vnode->fid.vid, vnode->fid.vnode, index, from, to);
 
-	/* We want to store information about how much of a page is altered in
-	 * page->private.
-	 */
-	BUILD_BUG_ON(PAGE_SIZE - 1 > __AFS_PAGE_PRIV_MASK && sizeof(page->private) < 8);
-
 	page = grab_cache_page_write_begin(mapping, index, flags);
 	if (!page)
 		return -ENOMEM;