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@xxxxxxxxxx>
> > > > ---
> > > > 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;
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
