From: Christoph Hellwig <hch@lst.de> To: Gao Xiang <hsiangkao@linux.alibaba.com> Cc: linux-erofs@lists.ozlabs.org, linux-fsdevel@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, Christoph Hellwig <hch@lst.de>, "Darrick J . Wong" <djwong@kernel.org>, Matthew Wilcox <willy@infradead.org>, Andreas Gruenbacher <andreas.gruenbacher@gmail.com> Subject: Re: [PATCH v6] iomap: support tail packing inline read Date: Thu, 22 Jul 2021 07:39:47 +0200 [thread overview] Message-ID: <20210722053947.GA28594@lst.de> (raw) In-Reply-To: <20210722031729.51628-1-hsiangkao@linux.alibaba.com> I think some of the language here is confusing - mostly about tail packing when we otherwise use inline data. Can you take a look at the version below? This mostly cleans up the terminology, adds a new helper to check the size, and removes the error on trying to write with a non-zero pos, as it can be trivially supported now. --- From 0f9c6ac6c2e372739b29195d25bebb8dd87e583a Mon Sep 17 00:00:00 2001 From: Gao Xiang <hsiangkao@linux.alibaba.com> Date: Thu, 22 Jul 2021 11:17:29 +0800 Subject: iomap: make inline data support more flexible Add support for offsets into the inline data page at iomap->inline_data to cater for the EROFS tailpackng case where a small data is stored right after the inode. Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com> --- fs/iomap/buffered-io.c | 35 ++++++++++++++++++----------------- fs/iomap/direct-io.c | 10 ++++++---- include/linux/iomap.h | 14 ++++++++++++++ 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 87ccb3438becd9..0597f5c186a33f 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -205,25 +205,29 @@ struct iomap_readpage_ctx { struct readahead_control *rac; }; -static void -iomap_read_inline_data(struct inode *inode, struct page *page, - struct iomap *iomap) +static int iomap_read_inline_data(struct inode *inode, struct page *page, + struct iomap *iomap, loff_t pos) { - size_t size = i_size_read(inode); + size_t size = iomap->length + iomap->offset - pos; void *addr; if (PageUptodate(page)) - return; + return PAGE_SIZE; - BUG_ON(page_has_private(page)); - BUG_ON(page->index); - BUG_ON(size > PAGE_SIZE - offset_in_page(iomap->inline_data)); + /* inline data must start page aligned in the file */ + if (WARN_ON_ONCE(offset_in_page(pos))) + return -EIO; + if (WARN_ON_ONCE(!iomap_inline_data_size_valid(iomap))) + return -EIO; + if (WARN_ON_ONCE(page_has_private(page))) + return -EIO; addr = kmap_atomic(page); - memcpy(addr, iomap->inline_data, size); + memcpy(addr, iomap_inline_buf(iomap, pos), size); memset(addr + size, 0, PAGE_SIZE - size); kunmap_atomic(addr); SetPageUptodate(page); + return PAGE_SIZE; } static inline bool iomap_block_needs_zeroing(struct inode *inode, @@ -246,11 +250,8 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, unsigned poff, plen; sector_t sector; - if (iomap->type == IOMAP_INLINE) { - WARN_ON_ONCE(pos); - iomap_read_inline_data(inode, page, iomap); - return PAGE_SIZE; - } + if (iomap->type == IOMAP_INLINE) + return iomap_read_inline_data(inode, page, iomap, pos); /* zero post-eof blocks as the page may be mapped */ iop = iomap_page_create(inode, page); @@ -618,14 +619,14 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, } if (srcmap->type == IOMAP_INLINE) - iomap_read_inline_data(inode, page, srcmap); + status = iomap_read_inline_data(inode, page, srcmap, pos); else if (iomap->flags & IOMAP_F_BUFFER_HEAD) status = __block_write_begin_int(page, pos, len, NULL, srcmap); else status = __iomap_write_begin(inode, pos, len, flags, page, srcmap); - if (unlikely(status)) + if (unlikely(status < 0)) goto out_unlock; *pagep = page; @@ -675,7 +676,7 @@ static size_t iomap_write_end_inline(struct inode *inode, struct page *page, flush_dcache_page(page); addr = kmap_atomic(page); - memcpy(iomap->inline_data + pos, addr + pos, copied); + memcpy(iomap_inline_buf(iomap, pos), addr + pos, copied); kunmap_atomic(addr); mark_inode_dirty(inode); diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 9398b8c31323b3..a6aaea2764a55f 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -378,23 +378,25 @@ iomap_dio_inline_actor(struct inode *inode, loff_t pos, loff_t length, struct iomap_dio *dio, struct iomap *iomap) { struct iov_iter *iter = dio->submit.iter; + void *dst = iomap_inline_buf(iomap, pos); size_t copied; - BUG_ON(pos + length > PAGE_SIZE - offset_in_page(iomap->inline_data)); + if (WARN_ON_ONCE(!iomap_inline_data_size_valid(iomap))) + return -EIO; if (dio->flags & IOMAP_DIO_WRITE) { loff_t size = inode->i_size; if (pos > size) - memset(iomap->inline_data + size, 0, pos - size); - copied = copy_from_iter(iomap->inline_data + pos, length, iter); + memset(iomap_inline_buf(iomap, size), 0, pos - size); + copied = copy_from_iter(dst, length, iter); if (copied) { if (pos + copied > size) i_size_write(inode, pos + copied); mark_inode_dirty(inode); } } else { - copied = copy_to_iter(iomap->inline_data + pos, length, iter); + copied = copy_to_iter(dst, length, iter); } dio->size += copied; return copied; diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 479c1da3e2211e..5efae7153912ed 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -97,6 +97,20 @@ iomap_sector(struct iomap *iomap, loff_t pos) return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; } +static inline void *iomap_inline_buf(const struct iomap *iomap, loff_t pos) +{ + return iomap->inline_data - iomap->offset + pos; +} + +/* + * iomap->inline_data is a potentially kmapped page, ensure it never crosseѕ a + * page boundary. + */ +static inline bool iomap_inline_data_size_valid(const struct iomap *iomap) +{ + return iomap->length <= PAGE_SIZE - offset_in_page(iomap->inline_data); +} + /* * When a filesystem sets page_ops in an iomap mapping it returns, page_prepare * and page_done will be called for each page written to. This only applies to -- 2.30.2
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de> To: Gao Xiang <hsiangkao@linux.alibaba.com> Cc: "Darrick J . Wong" <djwong@kernel.org>, Andreas Gruenbacher <andreas.gruenbacher@gmail.com>, LKML <linux-kernel@vger.kernel.org>, Matthew Wilcox <willy@infradead.org>, linux-fsdevel@vger.kernel.org, linux-erofs@lists.ozlabs.org, Christoph Hellwig <hch@lst.de> Subject: Re: [PATCH v6] iomap: support tail packing inline read Date: Thu, 22 Jul 2021 07:39:47 +0200 [thread overview] Message-ID: <20210722053947.GA28594@lst.de> (raw) In-Reply-To: <20210722031729.51628-1-hsiangkao@linux.alibaba.com> I think some of the language here is confusing - mostly about tail packing when we otherwise use inline data. Can you take a look at the version below? This mostly cleans up the terminology, adds a new helper to check the size, and removes the error on trying to write with a non-zero pos, as it can be trivially supported now. --- From 0f9c6ac6c2e372739b29195d25bebb8dd87e583a Mon Sep 17 00:00:00 2001 From: Gao Xiang <hsiangkao@linux.alibaba.com> Date: Thu, 22 Jul 2021 11:17:29 +0800 Subject: iomap: make inline data support more flexible Add support for offsets into the inline data page at iomap->inline_data to cater for the EROFS tailpackng case where a small data is stored right after the inode. Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com> --- fs/iomap/buffered-io.c | 35 ++++++++++++++++++----------------- fs/iomap/direct-io.c | 10 ++++++---- include/linux/iomap.h | 14 ++++++++++++++ 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 87ccb3438becd9..0597f5c186a33f 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -205,25 +205,29 @@ struct iomap_readpage_ctx { struct readahead_control *rac; }; -static void -iomap_read_inline_data(struct inode *inode, struct page *page, - struct iomap *iomap) +static int iomap_read_inline_data(struct inode *inode, struct page *page, + struct iomap *iomap, loff_t pos) { - size_t size = i_size_read(inode); + size_t size = iomap->length + iomap->offset - pos; void *addr; if (PageUptodate(page)) - return; + return PAGE_SIZE; - BUG_ON(page_has_private(page)); - BUG_ON(page->index); - BUG_ON(size > PAGE_SIZE - offset_in_page(iomap->inline_data)); + /* inline data must start page aligned in the file */ + if (WARN_ON_ONCE(offset_in_page(pos))) + return -EIO; + if (WARN_ON_ONCE(!iomap_inline_data_size_valid(iomap))) + return -EIO; + if (WARN_ON_ONCE(page_has_private(page))) + return -EIO; addr = kmap_atomic(page); - memcpy(addr, iomap->inline_data, size); + memcpy(addr, iomap_inline_buf(iomap, pos), size); memset(addr + size, 0, PAGE_SIZE - size); kunmap_atomic(addr); SetPageUptodate(page); + return PAGE_SIZE; } static inline bool iomap_block_needs_zeroing(struct inode *inode, @@ -246,11 +250,8 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, unsigned poff, plen; sector_t sector; - if (iomap->type == IOMAP_INLINE) { - WARN_ON_ONCE(pos); - iomap_read_inline_data(inode, page, iomap); - return PAGE_SIZE; - } + if (iomap->type == IOMAP_INLINE) + return iomap_read_inline_data(inode, page, iomap, pos); /* zero post-eof blocks as the page may be mapped */ iop = iomap_page_create(inode, page); @@ -618,14 +619,14 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, } if (srcmap->type == IOMAP_INLINE) - iomap_read_inline_data(inode, page, srcmap); + status = iomap_read_inline_data(inode, page, srcmap, pos); else if (iomap->flags & IOMAP_F_BUFFER_HEAD) status = __block_write_begin_int(page, pos, len, NULL, srcmap); else status = __iomap_write_begin(inode, pos, len, flags, page, srcmap); - if (unlikely(status)) + if (unlikely(status < 0)) goto out_unlock; *pagep = page; @@ -675,7 +676,7 @@ static size_t iomap_write_end_inline(struct inode *inode, struct page *page, flush_dcache_page(page); addr = kmap_atomic(page); - memcpy(iomap->inline_data + pos, addr + pos, copied); + memcpy(iomap_inline_buf(iomap, pos), addr + pos, copied); kunmap_atomic(addr); mark_inode_dirty(inode); diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 9398b8c31323b3..a6aaea2764a55f 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -378,23 +378,25 @@ iomap_dio_inline_actor(struct inode *inode, loff_t pos, loff_t length, struct iomap_dio *dio, struct iomap *iomap) { struct iov_iter *iter = dio->submit.iter; + void *dst = iomap_inline_buf(iomap, pos); size_t copied; - BUG_ON(pos + length > PAGE_SIZE - offset_in_page(iomap->inline_data)); + if (WARN_ON_ONCE(!iomap_inline_data_size_valid(iomap))) + return -EIO; if (dio->flags & IOMAP_DIO_WRITE) { loff_t size = inode->i_size; if (pos > size) - memset(iomap->inline_data + size, 0, pos - size); - copied = copy_from_iter(iomap->inline_data + pos, length, iter); + memset(iomap_inline_buf(iomap, size), 0, pos - size); + copied = copy_from_iter(dst, length, iter); if (copied) { if (pos + copied > size) i_size_write(inode, pos + copied); mark_inode_dirty(inode); } } else { - copied = copy_to_iter(iomap->inline_data + pos, length, iter); + copied = copy_to_iter(dst, length, iter); } dio->size += copied; return copied; diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 479c1da3e2211e..5efae7153912ed 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -97,6 +97,20 @@ iomap_sector(struct iomap *iomap, loff_t pos) return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; } +static inline void *iomap_inline_buf(const struct iomap *iomap, loff_t pos) +{ + return iomap->inline_data - iomap->offset + pos; +} + +/* + * iomap->inline_data is a potentially kmapped page, ensure it never crosseѕ a + * page boundary. + */ +static inline bool iomap_inline_data_size_valid(const struct iomap *iomap) +{ + return iomap->length <= PAGE_SIZE - offset_in_page(iomap->inline_data); +} + /* * When a filesystem sets page_ops in an iomap mapping it returns, page_prepare * and page_done will be called for each page written to. This only applies to -- 2.30.2
next prev parent reply other threads:[~2021-07-22 5:39 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-22 3:17 [PATCH v6] iomap: support tail packing inline read Gao Xiang 2021-07-22 3:17 ` Gao Xiang 2021-07-22 5:39 ` Christoph Hellwig [this message] 2021-07-22 5:39 ` Christoph Hellwig 2021-07-22 5:56 ` Gao Xiang 2021-07-22 5:56 ` Gao Xiang 2021-07-22 16:51 ` Darrick J. Wong 2021-07-22 16:51 ` Darrick J. Wong 2021-07-22 16:53 ` Christoph Hellwig 2021-07-22 16:53 ` Christoph Hellwig 2021-07-22 16:57 ` Matthew Wilcox 2021-07-22 16:57 ` Matthew Wilcox 2021-07-23 1:26 ` Gao Xiang 2021-07-23 1:26 ` Gao Xiang 2021-07-23 6:22 ` Huang Jianan 2021-07-23 6:22 ` Huang Jianan via Linux-erofs 2021-07-23 15:05 ` Matthew Wilcox 2021-07-23 15:05 ` Matthew Wilcox 2021-07-23 15:23 ` Gao Xiang 2021-07-23 15:23 ` Gao Xiang 2021-07-23 15:56 ` Matthew Wilcox 2021-07-23 16:24 ` Gao Xiang 2021-07-23 16:24 ` Gao Xiang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210722053947.GA28594@lst.de \ --to=hch@lst.de \ --cc=andreas.gruenbacher@gmail.com \ --cc=djwong@kernel.org \ --cc=hsiangkao@linux.alibaba.com \ --cc=linux-erofs@lists.ozlabs.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=willy@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.