In data=journal mode, page should be dirtied only when it has buffers
for checkpoint or it is writeably mapped. In the first case, we don't
need to do anything special. In the second case, page was already added
to the journal by ext4_page_mkwrite() and since transaction commit
writeprotects mapped pages again, page should be writeable (and thus
dirtied) only while it is part of the running transaction. So nothing
needs to be done either. The only special case is when someone pins the
page and uses this pin for modifying page data. So recognize this
special case and only then mark the page as having data that needs
adding to the journal.
Signed-off-by: Jan Kara <[email protected]>
---
fs/ext4/inode.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 13cab2a47f99..4c14aa1b9152 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3670,24 +3670,26 @@ const struct iomap_ops ext4_iomap_report_ops = {
};
/*
- * Whenever the folio is being dirtied, corresponding buffers should already
- * be attached to the transaction (we take care of this in ext4_page_mkwrite()
- * and ext4_write_begin()). However we cannot move buffers to dirty transaction
- * lists here because ->dirty_folio is called under VFS locks and the folio
- * is not necessarily locked.
- *
- * We cannot just dirty the folio and leave attached buffers clean, because the
- * buffers' dirty state is "definitive". We cannot just set the buffers dirty
- * or jbddirty because all the journalling code will explode.
- *
- * So what we do is to mark the folio "pending dirty" and next time writepage
- * is called, propagate that into the buffers appropriately.
+ * For data=journal mode, folio should be marked dirty only when it was
+ * writeably mapped. When that happens, it was already attached to the
+ * transaction and marked as jbddirty (we take care of this in
+ * ext4_page_mkwrite()). On transaction commit, we writeprotect page mappings
+ * so we should have nothing to do here, except for the case when someone
+ * had the page pinned and dirtied the page through this pin (e.g. by doing
+ * direct IO to it). In that case we'd need to attach buffers here to the
+ * transaction but we cannot due to lock ordering. We cannot just dirty the
+ * folio and leave attached buffers clean, because the buffers' dirty state is
+ * "definitive". We cannot just set the buffers dirty or jbddirty because all
+ * the journalling code will explode. So what we do is to mark the folio
+ * "pending dirty" and next time ext4_writepages() is called, attach buffers
+ * to the transaction appropriately.
*/
static bool ext4_journalled_dirty_folio(struct address_space *mapping,
struct folio *folio)
{
WARN_ON_ONCE(!folio_buffers(folio));
- folio_set_checked(folio);
+ if (folio_may_be_dma_pinned(folio))
+ folio_set_checked(folio);
return filemap_dirty_folio(mapping, folio);
}
--
2.35.3
Hi Jan,
I love your patch! Yet something to improve:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.2-rc3 next-20230111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/ext4-Update-stale-comment-about-write-constraints/20230111-234837
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/20230111154338.392-3-jack%40suse.cz
patch subject: [PATCH 3/7] ext4: Mark page for delayed dirtying only if it is pinned
config: alpha-randconfig-r003-20230110
compiler: alpha-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d96cf5a515c2cfae084227d197c80e53683c1cf8
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jan-Kara/ext4-Update-stale-comment-about-write-constraints/20230111-234837
git checkout d96cf5a515c2cfae084227d197c80e53683c1cf8
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
fs/ext4/inode.c: In function 'ext4_journalled_dirty_folio':
>> fs/ext4/inode.c:3691:13: error: implicit declaration of function 'folio_may_be_dma_pinned'; did you mean 'folio_maybe_dma_pinned'? [-Werror=implicit-function-declaration]
3691 | if (folio_may_be_dma_pinned(folio))
| ^~~~~~~~~~~~~~~~~~~~~~~
| folio_maybe_dma_pinned
cc1: some warnings being treated as errors
vim +3691 fs/ext4/inode.c
3671
3672 /*
3673 * For data=journal mode, folio should be marked dirty only when it was
3674 * writeably mapped. When that happens, it was already attached to the
3675 * transaction and marked as jbddirty (we take care of this in
3676 * ext4_page_mkwrite()). On transaction commit, we writeprotect page mappings
3677 * so we should have nothing to do here, except for the case when someone
3678 * had the page pinned and dirtied the page through this pin (e.g. by doing
3679 * direct IO to it). In that case we'd need to attach buffers here to the
3680 * transaction but we cannot due to lock ordering. We cannot just dirty the
3681 * folio and leave attached buffers clean, because the buffers' dirty state is
3682 * "definitive". We cannot just set the buffers dirty or jbddirty because all
3683 * the journalling code will explode. So what we do is to mark the folio
3684 * "pending dirty" and next time ext4_writepages() is called, attach buffers
3685 * to the transaction appropriately.
3686 */
3687 static bool ext4_journalled_dirty_folio(struct address_space *mapping,
3688 struct folio *folio)
3689 {
3690 WARN_ON_ONCE(!folio_buffers(folio));
> 3691 if (folio_may_be_dma_pinned(folio))
3692 folio_set_checked(folio);
3693 return filemap_dirty_folio(mapping, folio);
3694 }
3695
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Jan,
I love your patch! Yet something to improve:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.2-rc3 next-20230111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/ext4-Update-stale-comment-about-write-constraints/20230111-234837
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/20230111154338.392-3-jack%40suse.cz
patch subject: [PATCH 3/7] ext4: Mark page for delayed dirtying only if it is pinned
config: i386-randconfig-a002
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d96cf5a515c2cfae084227d197c80e53683c1cf8
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jan-Kara/ext4-Update-stale-comment-about-write-constraints/20230111-234837
git checkout d96cf5a515c2cfae084227d197c80e53683c1cf8
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> fs/ext4/inode.c:3691:6: error: implicit declaration of function 'folio_may_be_dma_pinned' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
if (folio_may_be_dma_pinned(folio))
^
fs/ext4/inode.c:3691:6: note: did you mean 'folio_maybe_dma_pinned'?
include/linux/mm.h:1562:20: note: 'folio_maybe_dma_pinned' declared here
static inline bool folio_maybe_dma_pinned(struct folio *folio)
^
1 error generated.
vim +/folio_may_be_dma_pinned +3691 fs/ext4/inode.c
3671
3672 /*
3673 * For data=journal mode, folio should be marked dirty only when it was
3674 * writeably mapped. When that happens, it was already attached to the
3675 * transaction and marked as jbddirty (we take care of this in
3676 * ext4_page_mkwrite()). On transaction commit, we writeprotect page mappings
3677 * so we should have nothing to do here, except for the case when someone
3678 * had the page pinned and dirtied the page through this pin (e.g. by doing
3679 * direct IO to it). In that case we'd need to attach buffers here to the
3680 * transaction but we cannot due to lock ordering. We cannot just dirty the
3681 * folio and leave attached buffers clean, because the buffers' dirty state is
3682 * "definitive". We cannot just set the buffers dirty or jbddirty because all
3683 * the journalling code will explode. So what we do is to mark the folio
3684 * "pending dirty" and next time ext4_writepages() is called, attach buffers
3685 * to the transaction appropriately.
3686 */
3687 static bool ext4_journalled_dirty_folio(struct address_space *mapping,
3688 struct folio *folio)
3689 {
3690 WARN_ON_ONCE(!folio_buffers(folio));
> 3691 if (folio_may_be_dma_pinned(folio))
3692 folio_set_checked(folio);
3693 return filemap_dirty_folio(mapping, folio);
3694 }
3695
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Jan,
I love your patch! Yet something to improve:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.2-rc3 next-20230111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/ext4-Update-stale-comment-about-write-constraints/20230111-234837
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/20230111154338.392-3-jack%40suse.cz
patch subject: [PATCH 3/7] ext4: Mark page for delayed dirtying only if it is pinned
config: i386-randconfig-a013
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d96cf5a515c2cfae084227d197c80e53683c1cf8
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jan-Kara/ext4-Update-stale-comment-about-write-constraints/20230111-234837
git checkout d96cf5a515c2cfae084227d197c80e53683c1cf8
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> fs/ext4/inode.c:3691:6: error: implicit declaration of function 'folio_may_be_dma_pinned' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
if (folio_may_be_dma_pinned(folio))
^
fs/ext4/inode.c:3691:6: note: did you mean 'folio_maybe_dma_pinned'?
include/linux/mm.h:1562:20: note: 'folio_maybe_dma_pinned' declared here
static inline bool folio_maybe_dma_pinned(struct folio *folio)
^
1 error generated.
vim +/folio_may_be_dma_pinned +3691 fs/ext4/inode.c
3671
3672 /*
3673 * For data=journal mode, folio should be marked dirty only when it was
3674 * writeably mapped. When that happens, it was already attached to the
3675 * transaction and marked as jbddirty (we take care of this in
3676 * ext4_page_mkwrite()). On transaction commit, we writeprotect page mappings
3677 * so we should have nothing to do here, except for the case when someone
3678 * had the page pinned and dirtied the page through this pin (e.g. by doing
3679 * direct IO to it). In that case we'd need to attach buffers here to the
3680 * transaction but we cannot due to lock ordering. We cannot just dirty the
3681 * folio and leave attached buffers clean, because the buffers' dirty state is
3682 * "definitive". We cannot just set the buffers dirty or jbddirty because all
3683 * the journalling code will explode. So what we do is to mark the folio
3684 * "pending dirty" and next time ext4_writepages() is called, attach buffers
3685 * to the transaction appropriately.
3686 */
3687 static bool ext4_journalled_dirty_folio(struct address_space *mapping,
3688 struct folio *folio)
3689 {
3690 WARN_ON_ONCE(!folio_buffers(folio));
> 3691 if (folio_may_be_dma_pinned(folio))
3692 folio_set_checked(folio);
3693 return filemap_dirty_folio(mapping, folio);
3694 }
3695
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Wed 11-01-23 16:43:27, Jan Kara wrote:
> In data=journal mode, page should be dirtied only when it has buffers
> for checkpoint or it is writeably mapped. In the first case, we don't
> need to do anything special. In the second case, page was already added
> to the journal by ext4_page_mkwrite() and since transaction commit
> writeprotects mapped pages again, page should be writeable (and thus
> dirtied) only while it is part of the running transaction. So nothing
> needs to be done either. The only special case is when someone pins the
> page and uses this pin for modifying page data. So recognize this
> special case and only then mark the page as having data that needs
> adding to the journal.
>
> Signed-off-by: Jan Kara <[email protected]>
...
> static bool ext4_journalled_dirty_folio(struct address_space *mapping,
> struct folio *folio)
> {
> WARN_ON_ONCE(!folio_buffers(folio));
> - folio_set_checked(folio);
> + if (folio_may_be_dma_pinned(folio))
Bah, this should be folio_maybe_dma_pinned(). I had it there and that's
what I've tested with but before submission I was laboring whether I should
really keep this change or not, had it deleted for a while and then
restored the change, and while doing that I've introduced this bug. :-|
I'll resend with this bug fixed but before doing that I'll wait a few days
whether someone has more comments.
Honza
> + folio_set_checked(folio);
> return filemap_dirty_folio(mapping, folio);
> }
>
> --
> 2.35.3
>
--
Jan Kara <[email protected]>
SUSE Labs, CR