diff mbox series

[v14,58/64] ceph: add encryption support to writepage

Message ID 20220427191314.222867-59-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series ceph+fscrypt: full support | expand

Commit Message

Jeff Layton April 27, 2022, 7:13 p.m. UTC
Allow writepage to issue encrypted writes. Extend out the requested size
and offset to cover complete blocks, and then encrypt and write them to
the OSDs.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

Comments

Xiubo Li May 5, 2022, 9:34 a.m. UTC | #1
On 4/28/22 3:13 AM, Jeff Layton wrote:
> Allow writepage to issue encrypted writes. Extend out the requested size
> and offset to cover complete blocks, and then encrypt and write them to
> the OSDs.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
>   1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index d65d431ec933..f54940fc96ee 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>   	loff_t page_off = page_offset(page);
>   	int err;
>   	loff_t len = thp_size(page);
> +	loff_t wlen;
>   	struct ceph_writeback_ctl ceph_wbc;
>   	struct ceph_osd_client *osdc = &fsc->client->osdc;
>   	struct ceph_osd_request *req;
>   	bool caching = ceph_is_cache_enabled(inode);
> +	struct page *bounce_page = NULL;
>   
>   	dout("writepage %p idx %lu\n", page, page->index);
>   
> @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>   
>   	if (ceph_wbc.i_size < page_off + len)
>   		len = ceph_wbc.i_size - page_off;
> +	if (IS_ENCRYPTED(inode))
> +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
>   
>   	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
>   	     inode, page, page->index, page_off, len, snapc, snapc->seq);
> @@ -629,24 +633,39 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>   	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
>   		fsc->write_congested = true;
>   
> -	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
> -				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
> -				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
> -				    true);
> +	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
> +				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
> +				    CEPH_OSD_FLAG_WRITE, snapc,
> +				    ceph_wbc.truncate_seq,
> +				    ceph_wbc.truncate_size, true);
>   	if (IS_ERR(req)) {
>   		redirty_page_for_writepage(wbc, page);
>   		return PTR_ERR(req);
>   	}
>   
> +	if (wlen < len)
> +		len = wlen;
> +
>   	set_page_writeback(page);
>   	if (caching)
>   		ceph_set_page_fscache(page);
>   	ceph_fscache_write_to_cache(inode, page_off, len, caching);
>   
> +	if (IS_ENCRYPTED(inode)) {
> +		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
> +								0, GFP_NOFS);
> +		if (IS_ERR(bounce_page)) {
> +			err = PTR_ERR(bounce_page);
> +			goto out;
> +		}
> +	}

Here IMO we should redirty the page instead of detaching the page's 
private data in 'out:' ?

-- Xiubo


>   	/* it may be a short write due to an object boundary */
>   	WARN_ON_ONCE(len > thp_size(page));
> -	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
> -	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
> +	osd_req_op_extent_osd_data_pages(req, 0,
> +			bounce_page ? &bounce_page : &page, wlen, 0,
> +			false, false);
> +	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
> +	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
>   
>   	req->r_mtime = inode->i_mtime;
>   	err = ceph_osdc_start_request(osdc, req, true);
> @@ -655,7 +674,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>   
>   	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
>   				  req->r_end_latency, len, err);
> -
> +	fscrypt_free_bounce_page(bounce_page);
> +out:
>   	ceph_osdc_put_request(req);
>   	if (err == 0)
>   		err = len;
Jeff Layton May 5, 2022, 10:53 a.m. UTC | #2
On Thu, 2022-05-05 at 17:34 +0800, Xiubo Li wrote:
> On 4/28/22 3:13 AM, Jeff Layton wrote:
> > Allow writepage to issue encrypted writes. Extend out the requested size
> > and offset to cover complete blocks, and then encrypt and write them to
> > the OSDs.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
> >   1 file changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index d65d431ec933..f54940fc96ee 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> >   	loff_t page_off = page_offset(page);
> >   	int err;
> >   	loff_t len = thp_size(page);
> > +	loff_t wlen;
> >   	struct ceph_writeback_ctl ceph_wbc;
> >   	struct ceph_osd_client *osdc = &fsc->client->osdc;
> >   	struct ceph_osd_request *req;
> >   	bool caching = ceph_is_cache_enabled(inode);
> > +	struct page *bounce_page = NULL;
> >   
> >   	dout("writepage %p idx %lu\n", page, page->index);
> >   
> > @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> >   
> >   	if (ceph_wbc.i_size < page_off + len)
> >   		len = ceph_wbc.i_size - page_off;
> > +	if (IS_ENCRYPTED(inode))
> > +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
> >   
> >   	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
> >   	     inode, page, page->index, page_off, len, snapc, snapc->seq);
> > @@ -629,24 +633,39 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> >   	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
> >   		fsc->write_congested = true;
> >   
> > -	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
> > -				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
> > -				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
> > -				    true);
> > +	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
> > +				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
> > +				    CEPH_OSD_FLAG_WRITE, snapc,
> > +				    ceph_wbc.truncate_seq,
> > +				    ceph_wbc.truncate_size, true);
> >   	if (IS_ERR(req)) {
> >   		redirty_page_for_writepage(wbc, page);
> >   		return PTR_ERR(req);
> >   	}
> >   
> > +	if (wlen < len)
> > +		len = wlen;
> > +
> >   	set_page_writeback(page);
> >   	if (caching)
> >   		ceph_set_page_fscache(page);
> >   	ceph_fscache_write_to_cache(inode, page_off, len, caching);
> >   
> > +	if (IS_ENCRYPTED(inode)) {
> > +		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
> > +								0, GFP_NOFS);
> > +		if (IS_ERR(bounce_page)) {
> > +			err = PTR_ERR(bounce_page);
> > +			goto out;
> > +		}
> > +	}
> 
> Here IMO we should redirty the page instead of detaching the page's 
> private data in 'out:' ?
> 
> -- Xiubo
> 
> 

Good catch. I think you're right. I'll fold the following delta into
this patch:

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index f54940fc96ee..b266656f2951 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -655,10 +655,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
                bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
                                                                0, GFP_NOFS);
                if (IS_ERR(bounce_page)) {
-                       err = PTR_ERR(bounce_page);
-                       goto out;
+                       redirty_page_for_writepage(wbc, page);
+                       end_page_writeback(page);
+                       return PTR_ERR(bounce_page);
                }
        }
+
        /* it may be a short write due to an object boundary */
        WARN_ON_ONCE(len > thp_size(page));
        osd_req_op_extent_osd_data_pages(req, 0,


> >   	/* it may be a short write due to an object boundary */
> >   	WARN_ON_ONCE(len > thp_size(page));
> > -	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
> > -	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
> > +	osd_req_op_extent_osd_data_pages(req, 0,
> > +			bounce_page ? &bounce_page : &page, wlen, 0,
> > +			false, false);
> > +	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
> > +	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
> >   
> >   	req->r_mtime = inode->i_mtime;
> >   	err = ceph_osdc_start_request(osdc, req, true);
> > @@ -655,7 +674,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> >   
> >   	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
> >   				  req->r_end_latency, len, err);
> > -
> > +	fscrypt_free_bounce_page(bounce_page);
> > +out:
> >   	ceph_osdc_put_request(req);
> >   	if (err == 0)
> >   		err = len;
>
Xiubo Li May 5, 2022, 11:05 a.m. UTC | #3
On 5/5/22 6:53 PM, Jeff Layton wrote:
> On Thu, 2022-05-05 at 17:34 +0800, Xiubo Li wrote:
>> On 4/28/22 3:13 AM, Jeff Layton wrote:
>>> Allow writepage to issue encrypted writes. Extend out the requested size
>>> and offset to cover complete blocks, and then encrypt and write them to
>>> the OSDs.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
>>>    1 file changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>>> index d65d431ec933..f54940fc96ee 100644
>>> --- a/fs/ceph/addr.c
>>> +++ b/fs/ceph/addr.c
>>> @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>    	loff_t page_off = page_offset(page);
>>>    	int err;
>>>    	loff_t len = thp_size(page);
>>> +	loff_t wlen;
>>>    	struct ceph_writeback_ctl ceph_wbc;
>>>    	struct ceph_osd_client *osdc = &fsc->client->osdc;
>>>    	struct ceph_osd_request *req;
>>>    	bool caching = ceph_is_cache_enabled(inode);
>>> +	struct page *bounce_page = NULL;
>>>    
>>>    	dout("writepage %p idx %lu\n", page, page->index);
>>>    
>>> @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>    
>>>    	if (ceph_wbc.i_size < page_off + len)
>>>    		len = ceph_wbc.i_size - page_off;
>>> +	if (IS_ENCRYPTED(inode))
>>> +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
>>>    
>>>    	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
>>>    	     inode, page, page->index, page_off, len, snapc, snapc->seq);
>>> @@ -629,24 +633,39 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>    	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
>>>    		fsc->write_congested = true;
>>>    
>>> -	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
>>> -				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
>>> -				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
>>> -				    true);
>>> +	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
>>> +				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
>>> +				    CEPH_OSD_FLAG_WRITE, snapc,
>>> +				    ceph_wbc.truncate_seq,
>>> +				    ceph_wbc.truncate_size, true);
>>>    	if (IS_ERR(req)) {
>>>    		redirty_page_for_writepage(wbc, page);
>>>    		return PTR_ERR(req);
>>>    	}
>>>    
>>> +	if (wlen < len)
>>> +		len = wlen;
>>> +
>>>    	set_page_writeback(page);
>>>    	if (caching)
>>>    		ceph_set_page_fscache(page);
>>>    	ceph_fscache_write_to_cache(inode, page_off, len, caching);
>>>    
>>> +	if (IS_ENCRYPTED(inode)) {
>>> +		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
>>> +								0, GFP_NOFS);
>>> +		if (IS_ERR(bounce_page)) {
>>> +			err = PTR_ERR(bounce_page);
>>> +			goto out;
>>> +		}
>>> +	}
>> Here IMO we should redirty the page instead of detaching the page's
>> private data in 'out:' ?
>>
>> -- Xiubo
>>
>>
> Good catch. I think you're right. I'll fold the following delta into
> this patch:
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index f54940fc96ee..b266656f2951 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -655,10 +655,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>                  bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
>                                                                  0, GFP_NOFS);
>                  if (IS_ERR(bounce_page)) {
> -                       err = PTR_ERR(bounce_page);
> -                       goto out;
> +                       redirty_page_for_writepage(wbc, page);
> +                       end_page_writeback(page);
> +                       return PTR_ERR(bounce_page);

You should also call ceph_osdc_put_request() too ?

-- Xiubo

>                  }
>          }
> +
>          /* it may be a short write due to an object boundary */
>          WARN_ON_ONCE(len > thp_size(page));
>          osd_req_op_extent_osd_data_pages(req, 0,
>
>
>>>    	/* it may be a short write due to an object boundary */
>>>    	WARN_ON_ONCE(len > thp_size(page));
>>> -	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
>>> -	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
>>> +	osd_req_op_extent_osd_data_pages(req, 0,
>>> +			bounce_page ? &bounce_page : &page, wlen, 0,
>>> +			false, false);
>>> +	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
>>> +	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
>>>    
>>>    	req->r_mtime = inode->i_mtime;
>>>    	err = ceph_osdc_start_request(osdc, req, true);
>>> @@ -655,7 +674,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>    
>>>    	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
>>>    				  req->r_end_latency, len, err);
>>> -
>>> +	fscrypt_free_bounce_page(bounce_page);
>>> +out:
>>>    	ceph_osdc_put_request(req);
>>>    	if (err == 0)
>>>    		err = len;
Jeff Layton May 5, 2022, 11:12 a.m. UTC | #4
On Thu, 2022-05-05 at 19:05 +0800, Xiubo Li wrote:
> On 5/5/22 6:53 PM, Jeff Layton wrote:
> > On Thu, 2022-05-05 at 17:34 +0800, Xiubo Li wrote:
> > > On 4/28/22 3:13 AM, Jeff Layton wrote:
> > > > Allow writepage to issue encrypted writes. Extend out the requested size
> > > > and offset to cover complete blocks, and then encrypt and write them to
> > > > the OSDs.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >    fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
> > > >    1 file changed, 27 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > > > index d65d431ec933..f54940fc96ee 100644
> > > > --- a/fs/ceph/addr.c
> > > > +++ b/fs/ceph/addr.c
> > > > @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> > > >    	loff_t page_off = page_offset(page);
> > > >    	int err;
> > > >    	loff_t len = thp_size(page);
> > > > +	loff_t wlen;
> > > >    	struct ceph_writeback_ctl ceph_wbc;
> > > >    	struct ceph_osd_client *osdc = &fsc->client->osdc;
> > > >    	struct ceph_osd_request *req;
> > > >    	bool caching = ceph_is_cache_enabled(inode);
> > > > +	struct page *bounce_page = NULL;
> > > >    
> > > >    	dout("writepage %p idx %lu\n", page, page->index);
> > > >    
> > > > @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> > > >    
> > > >    	if (ceph_wbc.i_size < page_off + len)
> > > >    		len = ceph_wbc.i_size - page_off;
> > > > +	if (IS_ENCRYPTED(inode))
> > > > +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
> > > >    
> > > >    	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
> > > >    	     inode, page, page->index, page_off, len, snapc, snapc->seq);
> > > > @@ -629,24 +633,39 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> > > >    	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
> > > >    		fsc->write_congested = true;
> > > >    
> > > > -	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
> > > > -				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
> > > > -				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
> > > > -				    true);
> > > > +	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
> > > > +				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
> > > > +				    CEPH_OSD_FLAG_WRITE, snapc,
> > > > +				    ceph_wbc.truncate_seq,
> > > > +				    ceph_wbc.truncate_size, true);
> > > >    	if (IS_ERR(req)) {
> > > >    		redirty_page_for_writepage(wbc, page);
> > > >    		return PTR_ERR(req);
> > > >    	}
> > > >    
> > > > +	if (wlen < len)
> > > > +		len = wlen;
> > > > +
> > > >    	set_page_writeback(page);
> > > >    	if (caching)
> > > >    		ceph_set_page_fscache(page);
> > > >    	ceph_fscache_write_to_cache(inode, page_off, len, caching);
> > > >    
> > > > +	if (IS_ENCRYPTED(inode)) {
> > > > +		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
> > > > +								0, GFP_NOFS);
> > > > +		if (IS_ERR(bounce_page)) {
> > > > +			err = PTR_ERR(bounce_page);
> > > > +			goto out;
> > > > +		}
> > > > +	}
> > > Here IMO we should redirty the page instead of detaching the page's
> > > private data in 'out:' ?
> > > 
> > > -- Xiubo
> > > 
> > > 
> > Good catch. I think you're right. I'll fold the following delta into
> > this patch:
> > 
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index f54940fc96ee..b266656f2951 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -655,10 +655,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> >                  bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
> >                                                                  0, GFP_NOFS);
> >                  if (IS_ERR(bounce_page)) {
> > -                       err = PTR_ERR(bounce_page);
> > -                       goto out;
> > +                       redirty_page_for_writepage(wbc, page);
> > +                       end_page_writeback(page);
> > +                       return PTR_ERR(bounce_page);
> 
> You should also call ceph_osdc_put_request() too ?
> 
> -- Xiubo
> 

Definitely:

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index f54940fc96ee..4dfa2892f3f5 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -655,10 +655,13 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
                bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
                                                                0, GFP_NOFS);
                if (IS_ERR(bounce_page)) {
-                       err = PTR_ERR(bounce_page);
-                       goto out;
+                       redirty_page_for_writepage(wbc, page);
+                       end_page_writeback(page);
+                       ceph_osdc_put_request(req);
+                       return PTR_ERR(bounce_page);
                }
        }
+
        /* it may be a short write due to an object boundary */
        WARN_ON_ONCE(len > thp_size(page));
        osd_req_op_extent_osd_data_pages(req, 0,


> >                  }
> >          }
> > +
> >          /* it may be a short write due to an object boundary */
> >          WARN_ON_ONCE(len > thp_size(page));
> >          osd_req_op_extent_osd_data_pages(req, 0,
> > 
> > 
> > > >    	/* it may be a short write due to an object boundary */
> > > >    	WARN_ON_ONCE(len > thp_size(page));
> > > > -	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
> > > > -	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
> > > > +	osd_req_op_extent_osd_data_pages(req, 0,
> > > > +			bounce_page ? &bounce_page : &page, wlen, 0,
> > > > +			false, false);
> > > > +	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
> > > > +	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
> > > >    
> > > >    	req->r_mtime = inode->i_mtime;
> > > >    	err = ceph_osdc_start_request(osdc, req, true);
> > > > @@ -655,7 +674,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> > > >    
> > > >    	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
> > > >    				  req->r_end_latency, len, err);
> > > > -
> > > > +	fscrypt_free_bounce_page(bounce_page);
> > > > +out:
> > > >    	ceph_osdc_put_request(req);
> > > >    	if (err == 0)
> > > >    		err = len;
>
Xiubo Li May 5, 2022, 11:27 a.m. UTC | #5
On 5/5/22 7:12 PM, Jeff Layton wrote:
> On Thu, 2022-05-05 at 19:05 +0800, Xiubo Li wrote:
>> On 5/5/22 6:53 PM, Jeff Layton wrote:
>>> On Thu, 2022-05-05 at 17:34 +0800, Xiubo Li wrote:
>>>> On 4/28/22 3:13 AM, Jeff Layton wrote:
>>>>> Allow writepage to issue encrypted writes. Extend out the requested size
>>>>> and offset to cover complete blocks, and then encrypt and write them to
>>>>> the OSDs.
>>>>>
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>> ---
>>>>>     fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
>>>>>     1 file changed, 27 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>>>>> index d65d431ec933..f54940fc96ee 100644
>>>>> --- a/fs/ceph/addr.c
>>>>> +++ b/fs/ceph/addr.c
>>>>> @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>>>     	loff_t page_off = page_offset(page);
>>>>>     	int err;
>>>>>     	loff_t len = thp_size(page);
>>>>> +	loff_t wlen;
>>>>>     	struct ceph_writeback_ctl ceph_wbc;
>>>>>     	struct ceph_osd_client *osdc = &fsc->client->osdc;
>>>>>     	struct ceph_osd_request *req;
>>>>>     	bool caching = ceph_is_cache_enabled(inode);
>>>>> +	struct page *bounce_page = NULL;
>>>>>     
>>>>>     	dout("writepage %p idx %lu\n", page, page->index);
>>>>>     
>>>>> @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>>>     
>>>>>     	if (ceph_wbc.i_size < page_off + len)
>>>>>     		len = ceph_wbc.i_size - page_off;
>>>>> +	if (IS_ENCRYPTED(inode))
>>>>> +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
>>>>>     
>>>>>     	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
>>>>>     	     inode, page, page->index, page_off, len, snapc, snapc->seq);
>>>>> @@ -629,24 +633,39 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>>>     	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
>>>>>     		fsc->write_congested = true;
>>>>>     
>>>>> -	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
>>>>> -				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
>>>>> -				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
>>>>> -				    true);
>>>>> +	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
>>>>> +				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
>>>>> +				    CEPH_OSD_FLAG_WRITE, snapc,
>>>>> +				    ceph_wbc.truncate_seq,
>>>>> +				    ceph_wbc.truncate_size, true);
>>>>>     	if (IS_ERR(req)) {
>>>>>     		redirty_page_for_writepage(wbc, page);
>>>>>     		return PTR_ERR(req);
>>>>>     	}
>>>>>     
>>>>> +	if (wlen < len)
>>>>> +		len = wlen;
>>>>> +
>>>>>     	set_page_writeback(page);
>>>>>     	if (caching)
>>>>>     		ceph_set_page_fscache(page);
>>>>>     	ceph_fscache_write_to_cache(inode, page_off, len, caching);
>>>>>     
>>>>> +	if (IS_ENCRYPTED(inode)) {
>>>>> +		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
>>>>> +								0, GFP_NOFS);
>>>>> +		if (IS_ERR(bounce_page)) {
>>>>> +			err = PTR_ERR(bounce_page);
>>>>> +			goto out;
>>>>> +		}
>>>>> +	}
>>>> Here IMO we should redirty the page instead of detaching the page's
>>>> private data in 'out:' ?
>>>>
>>>> -- Xiubo
>>>>
>>>>
>>> Good catch. I think you're right. I'll fold the following delta into
>>> this patch:
>>>
>>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>>> index f54940fc96ee..b266656f2951 100644
>>> --- a/fs/ceph/addr.c
>>> +++ b/fs/ceph/addr.c
>>> @@ -655,10 +655,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>                   bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
>>>                                                                   0, GFP_NOFS);
>>>                   if (IS_ERR(bounce_page)) {
>>> -                       err = PTR_ERR(bounce_page);
>>> -                       goto out;
>>> +                       redirty_page_for_writepage(wbc, page);
>>> +                       end_page_writeback(page);
>>> +                       return PTR_ERR(bounce_page);
>> You should also call ceph_osdc_put_request() too ?
>>
>> -- Xiubo
>>
> Definitely:
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index f54940fc96ee..4dfa2892f3f5 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -655,10 +655,13 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>                  bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
>                                                                  0, GFP_NOFS);
>                  if (IS_ERR(bounce_page)) {
> -                       err = PTR_ERR(bounce_page);
> -                       goto out;
> +                       redirty_page_for_writepage(wbc, page);
> +                       end_page_writeback(page);
> +                       ceph_osdc_put_request(req);
> +                       return PTR_ERR(bounce_page);
>                  }
>          }
> +
>          /* it may be a short write due to an object boundary */
>          WARN_ON_ONCE(len > thp_size(page));
>          osd_req_op_extent_osd_data_pages(req, 0,

Please remove the 'out' tag, it's not needed any more.

The others LGTM.

-- Xiubo


>
>>>                   }
>>>           }
>>> +
>>>           /* it may be a short write due to an object boundary */
>>>           WARN_ON_ONCE(len > thp_size(page));
>>>           osd_req_op_extent_osd_data_pages(req, 0,
>>>
>>>
>>>>>     	/* it may be a short write due to an object boundary */
>>>>>     	WARN_ON_ONCE(len > thp_size(page));
>>>>> -	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
>>>>> -	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
>>>>> +	osd_req_op_extent_osd_data_pages(req, 0,
>>>>> +			bounce_page ? &bounce_page : &page, wlen, 0,
>>>>> +			false, false);
>>>>> +	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
>>>>> +	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
>>>>>     
>>>>>     	req->r_mtime = inode->i_mtime;
>>>>>     	err = ceph_osdc_start_request(osdc, req, true);
>>>>> @@ -655,7 +674,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>>>     
>>>>>     	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
>>>>>     				  req->r_end_latency, len, err);
>>>>> -
>>>>> +	fscrypt_free_bounce_page(bounce_page);
>>>>> +out:
>>>>>     	ceph_osdc_put_request(req);
>>>>>     	if (err == 0)
>>>>>     		err = len;
Jeff Layton June 2, 2022, 4:08 p.m. UTC | #6
On Wed, 2022-04-27 at 15:13 -0400, Jeff Layton wrote:
> Allow writepage to issue encrypted writes. Extend out the requested size
> and offset to cover complete blocks, and then encrypt and write them to
> the OSDs.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index d65d431ec933..f54940fc96ee 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  	loff_t page_off = page_offset(page);
>  	int err;
>  	loff_t len = thp_size(page);
> +	loff_t wlen;
>  	struct ceph_writeback_ctl ceph_wbc;
>  	struct ceph_osd_client *osdc = &fsc->client->osdc;
>  	struct ceph_osd_request *req;
>  	bool caching = ceph_is_cache_enabled(inode);
> +	struct page *bounce_page = NULL;
>  
>  	dout("writepage %p idx %lu\n", page, page->index);
>  
> @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  
>  	if (ceph_wbc.i_size < page_off + len)
>  		len = ceph_wbc.i_size - page_off;
> +	if (IS_ENCRYPTED(inode))
> +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
>  

The above is buggy. We're only setting "wlen" in the encrypted case. You
would think that the compiler would catch that, but next usage of wlen
just passes a pointer to it to another function and that cloaks the
warning.

Fixed in the wip-fscrypt branch, but I'll hold off on re-posting the
series in case any other new bugs turn up.

>  	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
>  	     inode, page, page->index, page_off, len, snapc, snapc->seq);
> @@ -629,24 +633,39 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
>  		fsc->write_congested = true;
>  
> -	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
> -				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
> -				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
> -				    true);
> +	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
> +				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
> +				    CEPH_OSD_FLAG_WRITE, snapc,
> +				    ceph_wbc.truncate_seq,
> +				    ceph_wbc.truncate_size, true);
>  	if (IS_ERR(req)) {
>  		redirty_page_for_writepage(wbc, page);
>  		return PTR_ERR(req);
>  	}
>  
> +	if (wlen < len)
> +		len = wlen;
> +
>  	set_page_writeback(page);
>  	if (caching)
>  		ceph_set_page_fscache(page);
>  	ceph_fscache_write_to_cache(inode, page_off, len, caching);
>  
> +	if (IS_ENCRYPTED(inode)) {
> +		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
> +								0, GFP_NOFS);
> +		if (IS_ERR(bounce_page)) {
> +			err = PTR_ERR(bounce_page);
> +			goto out;
> +		}
> +	}
>  	/* it may be a short write due to an object boundary */
>  	WARN_ON_ONCE(len > thp_size(page));
> -	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
> -	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
> +	osd_req_op_extent_osd_data_pages(req, 0,
> +			bounce_page ? &bounce_page : &page, wlen, 0,
> +			false, false);
> +	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
> +	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
>  
>  	req->r_mtime = inode->i_mtime;
>  	err = ceph_osdc_start_request(osdc, req, true);
> @@ -655,7 +674,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  
>  	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
>  				  req->r_end_latency, len, err);
> -
> +	fscrypt_free_bounce_page(bounce_page);
> +out:
>  	ceph_osdc_put_request(req);
>  	if (err == 0)
>  		err = len;
Luís Henriques June 3, 2022, 9:17 a.m. UTC | #7
Jeff Layton <jlayton@kernel.org> writes:

> On Wed, 2022-04-27 at 15:13 -0400, Jeff Layton wrote:
>> Allow writepage to issue encrypted writes. Extend out the requested size
>> and offset to cover complete blocks, and then encrypt and write them to
>> the OSDs.
>> 
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>  fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
>>  1 file changed, 27 insertions(+), 7 deletions(-)
>> 
>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>> index d65d431ec933..f54940fc96ee 100644
>> --- a/fs/ceph/addr.c
>> +++ b/fs/ceph/addr.c
>> @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>  	loff_t page_off = page_offset(page);
>>  	int err;
>>  	loff_t len = thp_size(page);
>> +	loff_t wlen;
>>  	struct ceph_writeback_ctl ceph_wbc;
>>  	struct ceph_osd_client *osdc = &fsc->client->osdc;
>>  	struct ceph_osd_request *req;
>>  	bool caching = ceph_is_cache_enabled(inode);
>> +	struct page *bounce_page = NULL;
>>  
>>  	dout("writepage %p idx %lu\n", page, page->index);
>>  
>> @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>  
>>  	if (ceph_wbc.i_size < page_off + len)
>>  		len = ceph_wbc.i_size - page_off;
>> +	if (IS_ENCRYPTED(inode))
>> +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
>>  
>
> The above is buggy. We're only setting "wlen" in the encrypted case. You
> would think that the compiler would catch that, but next usage of wlen
> just passes a pointer to it to another function and that cloaks the
> warning.

Yikes!  That's indeed the sort of things we got used to have compilers
complaining about.  That must have been fun to figure this out.  Nice ;-)

Cheers,
Xiubo Li June 3, 2022, 11:33 a.m. UTC | #8
On 6/3/22 12:08 AM, Jeff Layton wrote:
> On Wed, 2022-04-27 at 15:13 -0400, Jeff Layton wrote:
>> Allow writepage to issue encrypted writes. Extend out the requested size
>> and offset to cover complete blocks, and then encrypt and write them to
>> the OSDs.
>>
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>   fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
>>   1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>> index d65d431ec933..f54940fc96ee 100644
>> --- a/fs/ceph/addr.c
>> +++ b/fs/ceph/addr.c
>> @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>   	loff_t page_off = page_offset(page);
>>   	int err;
>>   	loff_t len = thp_size(page);
>> +	loff_t wlen;
>>   	struct ceph_writeback_ctl ceph_wbc;
>>   	struct ceph_osd_client *osdc = &fsc->client->osdc;
>>   	struct ceph_osd_request *req;
>>   	bool caching = ceph_is_cache_enabled(inode);
>> +	struct page *bounce_page = NULL;
>>   
>>   	dout("writepage %p idx %lu\n", page, page->index);
>>   
>> @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>   
>>   	if (ceph_wbc.i_size < page_off + len)
>>   		len = ceph_wbc.i_size - page_off;
>> +	if (IS_ENCRYPTED(inode))
>> +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
>>   
> The above is buggy. We're only setting "wlen" in the encrypted case. You
> would think that the compiler would catch that, but next usage of wlen
> just passes a pointer to it to another function and that cloaks the
> warning.

Nice catch !


> Fixed in the wip-fscrypt branch, but I'll hold off on re-posting the
> series in case any other new bugs turn up.
>
>>   	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
>>   	     inode, page, page->index, page_off, len, snapc, snapc->seq);
>> @@ -629,24 +633,39 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>   	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
>>   		fsc->write_congested = true;
>>   
>> -	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
>> -				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
>> -				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
>> -				    true);
>> +	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
>> +				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
>> +				    CEPH_OSD_FLAG_WRITE, snapc,
>> +				    ceph_wbc.truncate_seq,
>> +				    ceph_wbc.truncate_size, true);
>>   	if (IS_ERR(req)) {
>>   		redirty_page_for_writepage(wbc, page);
>>   		return PTR_ERR(req);
>>   	}
>>   
>> +	if (wlen < len)
>> +		len = wlen;
>> +
>>   	set_page_writeback(page);
>>   	if (caching)
>>   		ceph_set_page_fscache(page);
>>   	ceph_fscache_write_to_cache(inode, page_off, len, caching);
>>   
>> +	if (IS_ENCRYPTED(inode)) {
>> +		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
>> +								0, GFP_NOFS);
>> +		if (IS_ERR(bounce_page)) {
>> +			err = PTR_ERR(bounce_page);
>> +			goto out;
>> +		}
>> +	}
>>   	/* it may be a short write due to an object boundary */
>>   	WARN_ON_ONCE(len > thp_size(page));
>> -	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
>> -	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
>> +	osd_req_op_extent_osd_data_pages(req, 0,
>> +			bounce_page ? &bounce_page : &page, wlen, 0,
>> +			false, false);
>> +	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
>> +	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
>>   
>>   	req->r_mtime = inode->i_mtime;
>>   	err = ceph_osdc_start_request(osdc, req, true);
>> @@ -655,7 +674,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>   
>>   	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
>>   				  req->r_end_latency, len, err);
>> -
>> +	fscrypt_free_bounce_page(bounce_page);
>> +out:
>>   	ceph_osdc_put_request(req);
>>   	if (err == 0)
>>   		err = len;
Jeff Layton June 3, 2022, 12:24 p.m. UTC | #9
On Fri, 2022-06-03 at 10:17 +0100, Luís Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > On Wed, 2022-04-27 at 15:13 -0400, Jeff Layton wrote:
> > > Allow writepage to issue encrypted writes. Extend out the requested size
> > > and offset to cover complete blocks, and then encrypt and write them to
> > > the OSDs.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
> > >  1 file changed, 27 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > > index d65d431ec933..f54940fc96ee 100644
> > > --- a/fs/ceph/addr.c
> > > +++ b/fs/ceph/addr.c
> > > @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> > >  	loff_t page_off = page_offset(page);
> > >  	int err;
> > >  	loff_t len = thp_size(page);
> > > +	loff_t wlen;
> > >  	struct ceph_writeback_ctl ceph_wbc;
> > >  	struct ceph_osd_client *osdc = &fsc->client->osdc;
> > >  	struct ceph_osd_request *req;
> > >  	bool caching = ceph_is_cache_enabled(inode);
> > > +	struct page *bounce_page = NULL;
> > >  
> > >  	dout("writepage %p idx %lu\n", page, page->index);
> > >  
> > > @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
> > >  
> > >  	if (ceph_wbc.i_size < page_off + len)
> > >  		len = ceph_wbc.i_size - page_off;
> > > +	if (IS_ENCRYPTED(inode))
> > > +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
> > >  
> > 
> > The above is buggy. We're only setting "wlen" in the encrypted case. You
> > would think that the compiler would catch that, but next usage of wlen
> > just passes a pointer to it to another function and that cloaks the
> > warning.
> 
> Yikes!  That's indeed the sort of things we got used to have compilers
> complaining about.  That must have been fun to figure this out.  Nice ;-)
> 

Yeah. I remember that some older versions of gcc would complain about
uninitialized vars when you passed a pointer to it to another function.
That went away a while back, which was good since it often fired on
false positives.

What would have been nice here would be for the compiler to notice that
wlen was inconsistently initialized before we passed the pointer to the
function. Not sure how hard that would be to catch though.
Xiubo Li June 3, 2022, 12:48 p.m. UTC | #10
On 6/3/22 8:24 PM, Jeff Layton wrote:
> On Fri, 2022-06-03 at 10:17 +0100, Luís Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> writes:
>>
>>> On Wed, 2022-04-27 at 15:13 -0400, Jeff Layton wrote:
>>>> Allow writepage to issue encrypted writes. Extend out the requested size
>>>> and offset to cover complete blocks, and then encrypt and write them to
>>>> the OSDs.
>>>>
>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> ---
>>>>   fs/ceph/addr.c | 34 +++++++++++++++++++++++++++-------
>>>>   1 file changed, 27 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>>>> index d65d431ec933..f54940fc96ee 100644
>>>> --- a/fs/ceph/addr.c
>>>> +++ b/fs/ceph/addr.c
>>>> @@ -586,10 +586,12 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>>   	loff_t page_off = page_offset(page);
>>>>   	int err;
>>>>   	loff_t len = thp_size(page);
>>>> +	loff_t wlen;
>>>>   	struct ceph_writeback_ctl ceph_wbc;
>>>>   	struct ceph_osd_client *osdc = &fsc->client->osdc;
>>>>   	struct ceph_osd_request *req;
>>>>   	bool caching = ceph_is_cache_enabled(inode);
>>>> +	struct page *bounce_page = NULL;
>>>>   
>>>>   	dout("writepage %p idx %lu\n", page, page->index);
>>>>   
>>>> @@ -621,6 +623,8 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>>>>   
>>>>   	if (ceph_wbc.i_size < page_off + len)
>>>>   		len = ceph_wbc.i_size - page_off;
>>>> +	if (IS_ENCRYPTED(inode))
>>>> +		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
>>>>   
>>> The above is buggy. We're only setting "wlen" in the encrypted case. You
>>> would think that the compiler would catch that, but next usage of wlen
>>> just passes a pointer to it to another function and that cloaks the
>>> warning.
>> Yikes!  That's indeed the sort of things we got used to have compilers
>> complaining about.  That must have been fun to figure this out.  Nice ;-)
>>
> Yeah. I remember that some older versions of gcc would complain about
> uninitialized vars when you passed a pointer to it to another function.
> That went away a while back, which was good since it often fired on
> false positives.
>
> What would have been nice here would be for the compiler to notice that
> wlen was inconsistently initialized before we passed the pointer to the
> function. Not sure how hard that would be to catch though.

I didn't compile this branch with the FSCRYPT being disabled yet 
recently, not sure whether will the compiler complains it then.
diff mbox series

Patch

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index d65d431ec933..f54940fc96ee 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -586,10 +586,12 @@  static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 	loff_t page_off = page_offset(page);
 	int err;
 	loff_t len = thp_size(page);
+	loff_t wlen;
 	struct ceph_writeback_ctl ceph_wbc;
 	struct ceph_osd_client *osdc = &fsc->client->osdc;
 	struct ceph_osd_request *req;
 	bool caching = ceph_is_cache_enabled(inode);
+	struct page *bounce_page = NULL;
 
 	dout("writepage %p idx %lu\n", page, page->index);
 
@@ -621,6 +623,8 @@  static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 
 	if (ceph_wbc.i_size < page_off + len)
 		len = ceph_wbc.i_size - page_off;
+	if (IS_ENCRYPTED(inode))
+		wlen = round_up(len, CEPH_FSCRYPT_BLOCK_SIZE);
 
 	dout("writepage %p page %p index %lu on %llu~%llu snapc %p seq %lld\n",
 	     inode, page, page->index, page_off, len, snapc, snapc->seq);
@@ -629,24 +633,39 @@  static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 	    CONGESTION_ON_THRESH(fsc->mount_options->congestion_kb))
 		fsc->write_congested = true;
 
-	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode), page_off, &len, 0, 1,
-				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE, snapc,
-				    ceph_wbc.truncate_seq, ceph_wbc.truncate_size,
-				    true);
+	req = ceph_osdc_new_request(osdc, &ci->i_layout, ceph_vino(inode),
+				    page_off, &wlen, 0, 1, CEPH_OSD_OP_WRITE,
+				    CEPH_OSD_FLAG_WRITE, snapc,
+				    ceph_wbc.truncate_seq,
+				    ceph_wbc.truncate_size, true);
 	if (IS_ERR(req)) {
 		redirty_page_for_writepage(wbc, page);
 		return PTR_ERR(req);
 	}
 
+	if (wlen < len)
+		len = wlen;
+
 	set_page_writeback(page);
 	if (caching)
 		ceph_set_page_fscache(page);
 	ceph_fscache_write_to_cache(inode, page_off, len, caching);
 
+	if (IS_ENCRYPTED(inode)) {
+		bounce_page = fscrypt_encrypt_pagecache_blocks(page, CEPH_FSCRYPT_BLOCK_SIZE,
+								0, GFP_NOFS);
+		if (IS_ERR(bounce_page)) {
+			err = PTR_ERR(bounce_page);
+			goto out;
+		}
+	}
 	/* it may be a short write due to an object boundary */
 	WARN_ON_ONCE(len > thp_size(page));
-	osd_req_op_extent_osd_data_pages(req, 0, &page, len, 0, false, false);
-	dout("writepage %llu~%llu (%llu bytes)\n", page_off, len, len);
+	osd_req_op_extent_osd_data_pages(req, 0,
+			bounce_page ? &bounce_page : &page, wlen, 0,
+			false, false);
+	dout("writepage %llu~%llu (%llu bytes, %sencrypted)\n",
+	     page_off, len, wlen, IS_ENCRYPTED(inode) ? "" : "not ");
 
 	req->r_mtime = inode->i_mtime;
 	err = ceph_osdc_start_request(osdc, req, true);
@@ -655,7 +674,8 @@  static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 
 	ceph_update_write_metrics(&fsc->mdsc->metric, req->r_start_latency,
 				  req->r_end_latency, len, err);
-
+	fscrypt_free_bounce_page(bounce_page);
+out:
 	ceph_osdc_put_request(req);
 	if (err == 0)
 		err = len;