| Message ID | 20180112141129.27507-3-chandan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 12, 2018 at 07:41:23PM +0530, Chandan Rajendra wrote: > @@ -1642,12 +1642,12 @@ EXPORT_SYMBOL(clean_bdev_aliases); > * constraints in mind (relevant mostly if some > * architecture has a slow bit-scan instruction) > */ > -static inline int block_size_bits(unsigned int blocksize) > +int block_size_bits(unsigned int blocksize) > { > return ilog2(blocksize); > } Could you move this to buffer.h instead please?
On Friday, January 12, 2018 8:08:23 PM IST Matthew Wilcox wrote: > On Fri, Jan 12, 2018 at 07:41:23PM +0530, Chandan Rajendra wrote: > > @@ -1642,12 +1642,12 @@ EXPORT_SYMBOL(clean_bdev_aliases); > > * constraints in mind (relevant mostly if some > > * architecture has a slow bit-scan instruction) > > */ > > -static inline int block_size_bits(unsigned int blocksize) > > +int block_size_bits(unsigned int blocksize) > > { > > return ilog2(blocksize); > > } > > Could you move this to buffer.h instead please? > > It just occured to me that I could use inode->i_blkbits instead of block_size_bits() inside the new function ext4_block_read_full_page(). Hence I will drop the above change from the next version of the patchset. Thanks for the review.
Hi Chandan, On Fri, Jan 12, 2018 at 07:41:23PM +0530, Chandan Rajendra wrote: > The functions end_buffer_async_read(), block_size_bits() and > create_page_buffers() will be needed by ext4 to implement encryption for > blocksize < pagesize scenario. > These all need to have EXPORT_SYMBOL() added, otherwise they won't be usable when ext4 is built as a module. block_size_bits() isn't actually needed as you can just use ->i_blkbits. For the remaining two functions being exposed you need to explain why they are *really* needed -- to create a version of block_read_full_page() that decrypts the data after reading it. Which is very ugly, and involves copy-and-pasting quite a bit of code. It's true that we've already effectively copy+pasted mpage_readpages() into ext4 (fs/ext4/readpage.c) for essentially the same reason, so your approach isn't really any worse. But did you consider instead cleaning things up by making the generic code support encryption? We now have the IS_ENCRYPTED() macro which allows generic code to efficiently tell whether the file is encrypted or not. I think the only problem is that the code in fs/crypto/ can be built as a module, so currently it can't be called directly by core code. So we could either change that and start requiring that fscrypt be built-in, or we could have the filesystem pass an (optional) decryption callback to the generic code. Or at the very least we could put the "encryption-aware" versions of mpages_readpages() and block_read_full_page() into fs/crypto/ rather than in fs/ext4/, so that they could be used by other filesystems in the future (although currently f2fs and ubifs won't need them). Eric
diff --git a/fs/buffer.c b/fs/buffer.c index 551b781..4360250 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -256,7 +256,7 @@ __find_get_block_slow(struct block_device *bdev, sector_t block) * 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_async_read(struct buffer_head *bh, int uptodate) { unsigned long flags; struct buffer_head *first; @@ -1642,12 +1642,12 @@ EXPORT_SYMBOL(clean_bdev_aliases); * constraints in mind (relevant mostly if some * architecture has a slow bit-scan instruction) */ -static inline int block_size_bits(unsigned int blocksize) +int block_size_bits(unsigned int blocksize) { return ilog2(blocksize); } -static struct buffer_head *create_page_buffers(struct page *page, struct inode *inode, unsigned int b_state) +struct buffer_head *create_page_buffers(struct page *page, struct inode *inode, unsigned int b_state) { BUG_ON(!PageLocked(page)); diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h index f1aed01..c26d16c 100644 --- a/include/linux/buffer_head.h +++ b/include/linux/buffer_head.h @@ -159,9 +159,13 @@ void set_bh_page(struct buffer_head *bh, int try_to_free_buffers(struct page *); struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size, bool retry); +int block_size_bits(unsigned int blocksize); void create_empty_buffers(struct page *, unsigned long, unsigned long b_state); +struct buffer_head *create_page_buffers(struct page *page, struct inode *inode, + unsigned int b_state); void end_buffer_read_sync(struct buffer_head *bh, int uptodate); +void end_buffer_async_read(struct buffer_head *bh, int uptodate); void end_buffer_write_sync(struct buffer_head *bh, int uptodate); void end_buffer_async_write(struct buffer_head *bh, int uptodate);
The functions end_buffer_async_read(), block_size_bits() and create_page_buffers() will be needed by ext4 to implement encryption for blocksize < pagesize scenario. Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> --- fs/buffer.c | 6 +++--- include/linux/buffer_head.h | 4 ++++ 2 files changed, 7 insertions(+), 3 deletions(-)