[PATCH] ceph: simplify ceph_sync_write() page_align, calculation

[PATCH] ceph: simplify ceph_sync_write() page_align, calculation

[Alex Elder]

In ceph_sync_write() there is some magic that computes page_align
for an osd request.  But a little analysis shows it can be
simplified.

First, we have:
 	io_align = pos & ~PAGE_MASK;
which is used here:
	page_align = (pos - io_align + buf_align) & ~PAGE_MASK;

Note (pos - io_align) simply rounds "pos" down to the nearest multiple
of the page size.

We also have:
 	buf_align = (unsigned long)data & ~PAGE_MASK;

Adding buf_align to that rounded-down "pos" value will stay within
the same page; the result will just be offset by the page offset for
the "data" pointer.  The final mask therefore leaves just the value
of "buf_align".

The same simplification can be done in striped_read().

One more simplification.  Note that the result of calc_pages_for()
is invariant of which page the offset starts in--the only thing that
matters is the offset within the starting page.  We will have
put the proper page offset to use into "page_align", so just use
that in calculating num_pages.

This resolves:
    http://tracker.ceph.com/issues/4166

Signed-off-by: Alex Elder <elder@inktank.com>
---
 fs/ceph/file.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 9c4325e..6123ad4 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -331,10 +331,7 @@ static int striped_read(struct inode *inode,
 	io_align = off & ~PAGE_MASK;

 more:
-	if (o_direct)
-		page_align = (pos - io_align + buf_align) & ~PAGE_MASK;
-	else
-		page_align = pos & ~PAGE_MASK;
+	page_align = o_direct ? buf_align : io_align;
 	this_len = left;
 	ret = ceph_osdc_readpages(&fsc->client->osdc, ceph_vino(inode),
 				  &ci->i_layout, pos, &this_len,
@@ -526,15 +523,10 @@ more:
 	io_align = pos & ~PAGE_MASK;
 	buf_align = (unsigned long)data & ~PAGE_MASK;
 	len = left;
-	if (file->f_flags & O_DIRECT) {
-		/* write from beginning of first page, regardless of
-		   io alignment */
-		page_align = (pos - io_align + buf_align) & ~PAGE_MASK;
-		num_pages = calc_pages_for((unsigned long)data, len);
-	} else {
-		page_align = pos & ~PAGE_MASK;
-		num_pages = calc_pages_for(pos, len);
-	}
+
+	/* write from beginning of first page, regardless of io alignment */
+	page_align = file->f_flags & O_DIRECT ? buf_align : io_align;
+	num_pages = calc_pages_for(page_align, len);
 	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
 				    ceph_vino(inode), pos, &len,
 				    CEPH_OSD_OP_WRITE, flags,
-- 
1.7.9.5

Re: [PATCH] ceph: simplify ceph_sync_write() page_align, calculation

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 02/16/2013 09:07 AM, Alex Elder wrote:
> In ceph_sync_write() there is some magic that computes page_align
> for an osd request.  But a little analysis shows it can be
> simplified.
>
> First, we have:
>   	io_align = pos & ~PAGE_MASK;
> which is used here:
> 	page_align = (pos - io_align + buf_align) & ~PAGE_MASK;
>
> Note (pos - io_align) simply rounds "pos" down to the nearest multiple
> of the page size.
>
> We also have:
>   	buf_align = (unsigned long)data & ~PAGE_MASK;
>
> Adding buf_align to that rounded-down "pos" value will stay within
> the same page; the result will just be offset by the page offset for
> the "data" pointer.  The final mask therefore leaves just the value
> of "buf_align".
>
> The same simplification can be done in striped_read().
>
> One more simplification.  Note that the result of calc_pages_for()
> is invariant of which page the offset starts in--the only thing that
> matters is the offset within the starting page.  We will have
> put the proper page offset to use into "page_align", so just use
> that in calculating num_pages.
>
> This resolves:
>      http://tracker.ceph.com/issues/4166
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   fs/ceph/file.c |   18 +++++-------------
>   1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 9c4325e..6123ad4 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -331,10 +331,7 @@ static int striped_read(struct inode *inode,
>   	io_align = off & ~PAGE_MASK;
>
>   more:
> -	if (o_direct)
> -		page_align = (pos - io_align + buf_align) & ~PAGE_MASK;
> -	else
> -		page_align = pos & ~PAGE_MASK;
> +	page_align = o_direct ? buf_align : io_align;
>   	this_len = left;
>   	ret = ceph_osdc_readpages(&fsc->client->osdc, ceph_vino(inode),
>   				  &ci->i_layout, pos, &this_len,
> @@ -526,15 +523,10 @@ more:
>   	io_align = pos & ~PAGE_MASK;
>   	buf_align = (unsigned long)data & ~PAGE_MASK;
>   	len = left;
> -	if (file->f_flags & O_DIRECT) {
> -		/* write from beginning of first page, regardless of
> -		   io alignment */
> -		page_align = (pos - io_align + buf_align) & ~PAGE_MASK;
> -		num_pages = calc_pages_for((unsigned long)data, len);
> -	} else {
> -		page_align = pos & ~PAGE_MASK;
> -		num_pages = calc_pages_for(pos, len);
> -	}
> +
> +	/* write from beginning of first page, regardless of io alignment */
> +	page_align = file->f_flags & O_DIRECT ? buf_align : io_align;
> +	num_pages = calc_pages_for(page_align, len);
>   	req = ceph_osdc_new_request(&fsc->client->osdc, &ci->i_layout,
>   				    ceph_vino(inode), pos, &len,
>   				    CEPH_OSD_OP_WRITE, flags,
>