Re: [f2fs-dev] [PATCH V2 07/13] Add decryption support for sub-pagesized blocks
Hi Eric,
On Tuesday, April 30, 2019 6:08:18 AM IST Eric Biggers wrote:
> On Sun, Apr 28, 2019 at 10:01:15AM +0530, Chandan Rajendra wrote:
> > To support decryption of sub-pagesized blocks this commit adds code to,
> > 1. Track buffer head in "struct read_callbacks_ctx".
> > 2. Pass buffer head argument to all read callbacks.
> > 3. In the corresponding endio, loop across all the blocks mapped by the
> >page, decrypting each block in turn.
> >
> > Signed-off-by: Chandan Rajendra
> > ---
> > fs/buffer.c| 83 +-
> > fs/crypto/bio.c| 50 +---
> > fs/crypto/crypto.c | 19 +++-
> > fs/f2fs/data.c | 2 +-
> > fs/mpage.c | 2 +-
> > fs/read_callbacks.c| 53 ++
> > include/linux/buffer_head.h| 1 +
> > include/linux/read_callbacks.h | 5 +-
> > 8 files changed, 154 insertions(+), 61 deletions(-)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index ce357602f471..f324727e24bb 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -45,6 +45,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);
> > @@ -245,11 +246,7 @@ __find_get_block_slow(struct block_device *bdev,
> > sector_t block)
> > return ret;
> > }
> >
> > -/*
> > - * I/O completion handler for block_read_full_page() - pages
> > - * which come unlocked at the end of I/O.
> > - */
> > -static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> > +void end_buffer_page_read(struct buffer_head *bh)
>
> I think __end_buffer_async_read() would be a better name, since the *page*
> isn't
> necessarily done yet.
>
> > {
> > unsigned long flags;
> > struct buffer_head *first;
> > @@ -257,17 +254,7 @@ static void end_buffer_async_read(struct buffer_head
> > *bh, int uptodate)
> > struct page *page;
> > int page_uptodate = 1;
> >
> > - BUG_ON(!buffer_async_read(bh));
> > -
> > page = bh->b_page;
> > - if (uptodate) {
> > - set_buffer_uptodate(bh);
> > - } else {
> > - clear_buffer_uptodate(bh);
> > - buffer_io_error(bh, ", async page read");
> > - SetPageError(page);
> > - }
> > -
> > /*
> > * Be _very_ careful from here on. Bad things can happen if
> > * two buffer heads end IO at almost the same time and both
> > @@ -305,6 +292,44 @@ static void end_buffer_async_read(struct buffer_head
> > *bh, int uptodate)
> > local_irq_restore(flags);
> > return;
> > }
> > +EXPORT_SYMBOL(end_buffer_page_read);
>
> No need for EXPORT_SYMBOL() here, as this is only called by built-in code.
>
> > +
> > +/*
> > + * I/O completion handler for block_read_full_page() - pages
> > + * which come unlocked at the end of I/O.
> > + */
>
> This comment is no longer correct. Change to something like:
>
> /*
> * I/O completion handler for block_read_full_page(). Pages are unlocked
> after
> * the I/O completes and the read callbacks (if any) have executed.
> */
>
> > +static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> > +{
> > + struct page *page;
> > +
> > + BUG_ON(!buffer_async_read(bh));
> > +
> > +#if defined(CONFIG_FS_ENCRYPTION) || defined(CONFIG_FS_VERITY)
> > + if (uptodate && bh->b_private) {
> > + struct read_callbacks_ctx *ctx = bh->b_private;
> > +
> > + read_callbacks(ctx);
> > + return;
> > + }
> > +
> > + if (bh->b_private) {
> > + struct read_callbacks_ctx *ctx = bh->b_private;
> > +
> > + WARN_ON(uptodate);
> > + put_read_callbacks_ctx(ctx);
> > + }
> > +#endif
>
> These details should be handled in read_callbacks code, not here. AFAICS, all
> you need is a function read_callbacks_end_bh() that returns a bool indicating
> whether it handled the buffer_head or not:
>
> static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> {
> BUG_ON(!buffer_async_read(bh));
>
> if (read_callbacks_end_bh(bh, uptodate))
> return;
>
> page = bh->b_page;
> ...
> }
>
> Then read_callbacks_end_bh() would check ->b_private and uptodate, and call
> read_callbacks() or put_read_callbacks_ctx() as appropriate. When
> CONFIG_FS_READ_CALLBACKS=n it would be a stub that always returns false.
>
> > + page = bh->b_page;
> [...]
>
> > }
> > @@ -2292,11 +2323,21 @@ int block_read_full_page(struct page *page,
> > get_block_t *get_block)
> > * the underlying blockdev brought it uptodate (the sct fix).
> > */
> > for (i = 0; i < nr; i++) {
> > - bh = arr[i];
> > - if (buffer_uptodate(bh))
> > + bh = arr[i].bh;
> > + if (buffer_uptodate(bh)) {
> > end_buffer_async_read(bh, 1);
> > - else
> > + } else {
> > +#if
Re: [f2fs-dev] [PATCH V2 07/13] Add decryption support for sub-pagesized blocks
On Sun, Apr 28, 2019 at 10:01:15AM +0530, Chandan Rajendra wrote:
> To support decryption of sub-pagesized blocks this commit adds code to,
> 1. Track buffer head in "struct read_callbacks_ctx".
> 2. Pass buffer head argument to all read callbacks.
> 3. In the corresponding endio, loop across all the blocks mapped by the
>page, decrypting each block in turn.
>
> Signed-off-by: Chandan Rajendra
> ---
> fs/buffer.c| 83 +-
> fs/crypto/bio.c| 50 +---
> fs/crypto/crypto.c | 19 +++-
> fs/f2fs/data.c | 2 +-
> fs/mpage.c | 2 +-
> fs/read_callbacks.c| 53 ++
> include/linux/buffer_head.h| 1 +
> include/linux/read_callbacks.h | 5 +-
> 8 files changed, 154 insertions(+), 61 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index ce357602f471..f324727e24bb 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -45,6 +45,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);
> @@ -245,11 +246,7 @@ __find_get_block_slow(struct block_device *bdev,
> sector_t block)
> return ret;
> }
>
> -/*
> - * I/O completion handler for block_read_full_page() - pages
> - * which come unlocked at the end of I/O.
> - */
> -static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> +void end_buffer_page_read(struct buffer_head *bh)
I think __end_buffer_async_read() would be a better name, since the *page* isn't
necessarily done yet.
> {
> unsigned long flags;
> struct buffer_head *first;
> @@ -257,17 +254,7 @@ static void end_buffer_async_read(struct buffer_head
> *bh, int uptodate)
> struct page *page;
> int page_uptodate = 1;
>
> - BUG_ON(!buffer_async_read(bh));
> -
> page = bh->b_page;
> - if (uptodate) {
> - set_buffer_uptodate(bh);
> - } else {
> - clear_buffer_uptodate(bh);
> - buffer_io_error(bh, ", async page read");
> - SetPageError(page);
> - }
> -
> /*
>* Be _very_ careful from here on. Bad things can happen if
>* two buffer heads end IO at almost the same time and both
> @@ -305,6 +292,44 @@ static void end_buffer_async_read(struct buffer_head
> *bh, int uptodate)
> local_irq_restore(flags);
> return;
> }
> +EXPORT_SYMBOL(end_buffer_page_read);
No need for EXPORT_SYMBOL() here, as this is only called by built-in code.
> +
> +/*
> + * I/O completion handler for block_read_full_page() - pages
> + * which come unlocked at the end of I/O.
> + */
This comment is no longer correct. Change to something like:
/*
* I/O completion handler for block_read_full_page(). Pages are unlocked after
* the I/O completes and the read callbacks (if any) have executed.
*/
> +static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> +{
> + struct page *page;
> +
> + BUG_ON(!buffer_async_read(bh));
> +
> +#if defined(CONFIG_FS_ENCRYPTION) || defined(CONFIG_FS_VERITY)
> + if (uptodate && bh->b_private) {
> + struct read_callbacks_ctx *ctx = bh->b_private;
> +
> + read_callbacks(ctx);
> + return;
> + }
> +
> + if (bh->b_private) {
> + struct read_callbacks_ctx *ctx = bh->b_private;
> +
> + WARN_ON(uptodate);
> + put_read_callbacks_ctx(ctx);
> + }
> +#endif
These details should be handled in read_callbacks code, not here. AFAICS, all
you need is a function read_callbacks_end_bh() that returns a bool indicating
whether it handled the buffer_head or not:
static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
{
BUG_ON(!buffer_async_read(bh));
if (read_callbacks_end_bh(bh, uptodate))
return;
page = bh->b_page;
...
}
Then read_callbacks_end_bh() would check ->b_private and uptodate, and call
read_callbacks() or put_read_callbacks_ctx() as appropriate. When
CONFIG_FS_READ_CALLBACKS=n it would be a stub that always returns false.
> + page = bh->b_page;
[...]
> }
> @@ -2292,11 +2323,21 @@ int block_read_full_page(struct page *page,
> get_block_t *get_block)
>* the underlying blockdev brought it uptodate (the sct fix).
>*/
> for (i = 0; i < nr; i++) {
> - bh = arr[i];
> - if (buffer_uptodate(bh))
> + bh = arr[i].bh;
> + if (buffer_uptodate(bh)) {
> end_buffer_async_read(bh, 1);
> - else
> + } else {
> +#if defined(CONFIG_FS_ENCRYPTION) || defined(CONFIG_FS_VERITY)
> + struct read_callbacks_ctx *ctx;
> +
> + ctx = get_read_callbacks_ctx(inode, NULL, bh,
> arr[i].blk_nr);
> + if (WARN_ON(IS_ERR(ctx))) {
> +
