I don't really want to be working on this ... I'm a bit outside my zone
of knowledge here, and I'd much rather be working on folio conversions.
It mostly works, but a full xfstests run dies, probably from a memory
leak.
Matthew Wilcox (Oracle) (3):
quota: Prevent memory allocation recursion while holding dq_lock
quota: Support using the page cache for quota files
ext4: Use generic_quota_read()
fs/ext4/super.c | 81 ++++++++++------------------------------
fs/quota/dquot.c | 77 ++++++++++++++++++++++++++++++++++++++
include/linux/fs.h | 2 +
include/linux/quotaops.h | 1 +
4 files changed, 100 insertions(+), 61 deletions(-)
--
2.35.1
Quota files are usually cached in the buffer cache of the block device.
These support functions allow a filesystem to cache quota files in their
page cache instead which is more efficient.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/quota/dquot.c | 68 ++++++++++++++++++++++++++++++++++++++++
include/linux/fs.h | 2 ++
include/linux/quotaops.h | 1 +
3 files changed, 71 insertions(+)
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index cdb22d6d7488..ef9aeae802c7 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -59,6 +59,7 @@
#include <linux/fs.h>
#include <linux/mount.h>
#include <linux/mm.h>
+#include <linux/pagemap.h>
#include <linux/time.h>
#include <linux/types.h>
#include <linux/string.h>
@@ -73,6 +74,7 @@
#include <linux/proc_fs.h>
#include <linux/security.h>
#include <linux/sched.h>
+#include <linux/sched/mm.h>
#include <linux/cred.h>
#include <linux/kmod.h>
#include <linux/namei.h>
@@ -2161,6 +2163,72 @@ const struct dquot_operations dquot_operations = {
};
EXPORT_SYMBOL(dquot_operations);
+ssize_t generic_quota_read(struct super_block *sb, int type, char *data,
+ size_t len, loff_t pos)
+{
+ struct inode *inode = sb_dqopt(sb)->files[type];
+ struct address_space *mapping = inode->i_mapping;
+ size_t toread;
+ pgoff_t index;
+ loff_t i_size = i_size_read(inode);
+
+ if (pos > i_size)
+ return 0;
+ if (pos + len > i_size)
+ len = i_size - pos;
+ toread = len;
+ index = pos / PAGE_SIZE;
+
+ while (toread > 0) {
+ struct folio *folio = read_mapping_folio(mapping, index, NULL);
+ size_t tocopy = min(toread, PAGE_SIZE - offset_in_page(pos));
+ void *src;
+
+ if (folio == ERR_PTR(-ENOMEM)) {
+ memalloc_retry_wait(GFP_NOFS);
+ continue;
+ } else if (IS_ERR(folio))
+ return PTR_ERR(folio);
+
+ src = kmap_local_folio(folio, offset_in_folio(folio, pos));
+ memcpy(data, src, tocopy);
+ kunmap_local(src);
+ folio_put(folio);
+
+ toread -= tocopy;
+ data += tocopy;
+ pos += tocopy;
+ index++;
+ }
+ return len;
+}
+EXPORT_SYMBOL(generic_quota_read);
+
+int generic_quota_sync(struct super_block *sb, int type)
+{
+ struct quota_info *dqopt = sb_dqopt(sb);
+ int i, ret;
+
+ ret = dquot_writeback_dquots(sb, type);
+ if (ret)
+ return ret;
+ if (dqopt->flags & DQUOT_QUOTA_SYS_FILE)
+ return 0;
+
+ for (i = 0; i < MAXQUOTAS; i++) {
+ if (type != -1 && type != i)
+ continue;
+ if (!sb_has_quota_active(sb, i))
+ continue;
+ ret = write_inode_now(dqopt->files[i], true);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(generic_quota_sync);
+
/*
* Generic helper for ->open on filesystems supporting disk quotas.
*/
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9ad5e3520fae..2e798fc4c118 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2449,6 +2449,8 @@ struct super_block *sget(struct file_system_type *type,
int (*test)(struct super_block *,void *),
int (*set)(struct super_block *,void *),
int flags, void *data);
+ssize_t generic_quota_read(struct super_block *sb, int type, char *data,
+ size_t len, loff_t pos);
/* Alas, no aliases. Too much hassle with bringing module.h everywhere */
#define fops_get(fops) \
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index a0f6668924d3..fe12b04948f6 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -105,6 +105,7 @@ int dquot_quota_on_mount(struct super_block *sb, char *qf_name,
int dquot_quota_off(struct super_block *sb, int type);
int dquot_writeback_dquots(struct super_block *sb, int type);
int dquot_quota_sync(struct super_block *sb, int type);
+int generic_quota_sync(struct super_block *sb, int type);
int dquot_get_state(struct super_block *sb, struct qc_state *state);
int dquot_set_dqinfo(struct super_block *sb, int type, struct qc_info *ii);
int dquot_get_dqblk(struct super_block *sb, struct kqid id,
--
2.35.1
The comment about the page cache is rather stale; the buffer cache will
read into the page cache if the buffer isn't present, and the page cache
will not take any locks if the page is present.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/ext4/super.c | 81 ++++++++++++-------------------------------------
1 file changed, 20 insertions(+), 61 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 450c918d68fc..1780649ed224 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1506,8 +1506,6 @@ static int ext4_mark_dquot_dirty(struct dquot *dquot);
static int ext4_write_info(struct super_block *sb, int type);
static int ext4_quota_on(struct super_block *sb, int type, int format_id,
const struct path *path);
-static ssize_t ext4_quota_read(struct super_block *sb, int type, char *data,
- size_t len, loff_t off);
static ssize_t ext4_quota_write(struct super_block *sb, int type,
const char *data, size_t len, loff_t off);
static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
@@ -1535,7 +1533,7 @@ static const struct dquot_operations ext4_quota_operations = {
static const struct quotactl_ops ext4_qctl_operations = {
.quota_on = ext4_quota_on,
.quota_off = ext4_quota_off,
- .quota_sync = dquot_quota_sync,
+ .quota_sync = generic_quota_sync,
.get_state = dquot_get_state,
.set_info = dquot_set_dqinfo,
.get_dqblk = dquot_get_dqblk,
@@ -1559,7 +1557,7 @@ static const struct super_operations ext4_sops = {
.statfs = ext4_statfs,
.show_options = ext4_show_options,
#ifdef CONFIG_QUOTA
- .quota_read = ext4_quota_read,
+ .quota_read = generic_quota_read,
.quota_write = ext4_quota_write,
.get_dquots = ext4_get_dquots,
#endif
@@ -6856,55 +6854,15 @@ static int ext4_quota_off(struct super_block *sb, int type)
return dquot_quota_off(sb, type);
}
-/* Read data from quotafile - avoid pagecache and such because we cannot afford
- * acquiring the locks... As quota files are never truncated and quota code
- * itself serializes the operations (and no one else should touch the files)
- * we don't have to be afraid of races */
-static ssize_t ext4_quota_read(struct super_block *sb, int type, char *data,
- size_t len, loff_t off)
-{
- struct inode *inode = sb_dqopt(sb)->files[type];
- ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
- int offset = off & (sb->s_blocksize - 1);
- int tocopy;
- size_t toread;
- struct buffer_head *bh;
- loff_t i_size = i_size_read(inode);
-
- if (off > i_size)
- return 0;
- if (off+len > i_size)
- len = i_size-off;
- toread = len;
- while (toread > 0) {
- tocopy = sb->s_blocksize - offset < toread ?
- sb->s_blocksize - offset : toread;
- bh = ext4_bread(NULL, inode, blk, 0);
- if (IS_ERR(bh))
- return PTR_ERR(bh);
- if (!bh) /* A hole? */
- memset(data, 0, tocopy);
- else
- memcpy(data, bh->b_data+offset, tocopy);
- brelse(bh);
- offset = 0;
- toread -= tocopy;
- data += tocopy;
- blk++;
- }
- return len;
-}
-
/* Write to quotafile (we know the transaction is already started and has
* enough credits) */
static ssize_t ext4_quota_write(struct super_block *sb, int type,
const char *data, size_t len, loff_t off)
{
struct inode *inode = sb_dqopt(sb)->files[type];
- ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
- int err = 0, err2 = 0, offset = off & (sb->s_blocksize - 1);
- int retries = 0;
- struct buffer_head *bh;
+ int err = 0, offset = off & (sb->s_blocksize - 1);
+ struct buffer_head *bh, *head;
+ struct folio *folio;
handle_t *handle = journal_current_handle();
if (!handle) {
@@ -6924,20 +6882,21 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
return -EIO;
}
- do {
- bh = ext4_bread(handle, inode, blk,
- EXT4_GET_BLOCKS_CREATE |
- EXT4_GET_BLOCKS_METADATA_NOFAIL);
- } while (PTR_ERR(bh) == -ENOSPC &&
- ext4_should_retry_alloc(inode->i_sb, &retries));
- if (IS_ERR(bh))
- return PTR_ERR(bh);
- if (!bh)
+ folio = read_mapping_folio(inode->i_mapping, off / PAGE_SIZE, NULL);
+ if (IS_ERR(folio))
+ return PTR_ERR(folio);
+ head = folio_buffers(folio);
+ if (!head)
+ head = alloc_page_buffers(&folio->page, sb->s_blocksize, false);
+ if (!head)
goto out;
+ bh = head;
+ while ((bh_offset(bh) + sb->s_blocksize) <= (off % PAGE_SIZE))
+ bh = bh->b_this_page;
BUFFER_TRACE(bh, "get write access");
err = ext4_journal_get_write_access(handle, sb, bh, EXT4_JTR_NONE);
if (err) {
- brelse(bh);
+ folio_put(folio);
return err;
}
lock_buffer(bh);
@@ -6945,14 +6904,14 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
flush_dcache_page(bh->b_page);
unlock_buffer(bh);
err = ext4_handle_dirty_metadata(handle, NULL, bh);
- brelse(bh);
out:
+ folio_put(folio);
+ if (err)
+ return err;
if (inode->i_size < off + len) {
i_size_write(inode, off + len);
EXT4_I(inode)->i_disksize = inode->i_size;
- err2 = ext4_mark_inode_dirty(handle, inode);
- if (unlikely(err2 && !err))
- err = err2;
+ err = ext4_mark_inode_dirty(handle, inode);
}
return err ? err : len;
}
--
2.35.1
Hi "Matthew,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on jack-fs/for_next linus/master v5.19-rc1 next-20220603]
[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]
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/Cache-quota-files-in-the-page-cache/20220606-021629
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: mips-mtx1_defconfig (https://download.01.org/0day-ci/archive/20220606/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b92436efcb7813fc481b30f2593a4907568d917a)
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
# install mips cross compiling tool for clang build
# apt-get install binutils-mipsel-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/a7ff347e002ef476c8c116f30858f83529638a9b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/Cache-quota-files-in-the-page-cache/20220606-021629
git checkout a7ff347e002ef476c8c116f30858f83529638a9b
# 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=mips SHELL=/bin/bash fs/quota/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> fs/quota/dquot.c:2184:19: warning: comparison of distinct pointer types ('typeof (toread) *' (aka 'unsigned int *') and 'typeof (((1UL) << 12) - ((unsigned long)(pos) & ~(~((1 << 12) - 1)))) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
size_t tocopy = min(toread, PAGE_SIZE - offset_in_page(pos));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:45:19: note: expanded from macro 'min'
#define min(x, y) __careful_cmp(x, y, <)
^~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~~~~~~~
include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~~~~~~~
include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
1 warning generated.
vim +2184 fs/quota/dquot.c
2165
2166 ssize_t generic_quota_read(struct super_block *sb, int type, char *data,
2167 size_t len, loff_t pos)
2168 {
2169 struct inode *inode = sb_dqopt(sb)->files[type];
2170 struct address_space *mapping = inode->i_mapping;
2171 size_t toread;
2172 pgoff_t index;
2173 loff_t i_size = i_size_read(inode);
2174
2175 if (pos > i_size)
2176 return 0;
2177 if (pos + len > i_size)
2178 len = i_size - pos;
2179 toread = len;
2180 index = pos / PAGE_SIZE;
2181
2182 while (toread > 0) {
2183 struct folio *folio = read_mapping_folio(mapping, index, NULL);
> 2184 size_t tocopy = min(toread, PAGE_SIZE - offset_in_page(pos));
2185 void *src;
2186
2187 if (folio == ERR_PTR(-ENOMEM)) {
2188 memalloc_retry_wait(GFP_NOFS);
2189 continue;
2190 } else if (IS_ERR(folio))
2191 return PTR_ERR(folio);
2192
2193 src = kmap_local_folio(folio, offset_in_folio(folio, pos));
2194 memcpy(data, src, tocopy);
2195 kunmap_local(src);
2196 folio_put(folio);
2197
2198 toread -= tocopy;
2199 data += tocopy;
2200 pos += tocopy;
2201 index++;
2202 }
2203 return len;
2204 }
2205 EXPORT_SYMBOL(generic_quota_read);
2206
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi "Matthew,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on jack-fs/for_next linus/master]
[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]
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/Cache-quota-files-in-the-page-cache/20220606-021629
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: riscv-randconfig-c006-20220605 (https://download.01.org/0day-ci/archive/20220606/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b92436efcb7813fc481b30f2593a4907568d917a)
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
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/a7ff347e002ef476c8c116f30858f83529638a9b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/Cache-quota-files-in-the-page-cache/20220606-021629
git checkout a7ff347e002ef476c8c116f30858f83529638a9b
# 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=riscv SHELL=/bin/bash fs/quota/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> fs/quota/dquot.c:2184:19: warning: comparison of distinct pointer types ('typeof (toread) *' (aka 'unsigned int *') and 'typeof (((1UL) << (12)) - ((unsigned long)(pos) & ~(~(((1UL) << (12)) - 1)))) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
size_t tocopy = min(toread, PAGE_SIZE - offset_in_page(pos));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:45:19: note: expanded from macro 'min'
#define min(x, y) __careful_cmp(x, y, <)
^~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~~~~~~~
include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~~~~~~~
include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
error: A dwo section may not contain relocations
fatal error: too many errors emitted, stopping now [-ferror-limit=]
1 warning and 20 errors generated.
vim +2184 fs/quota/dquot.c
2165
2166 ssize_t generic_quota_read(struct super_block *sb, int type, char *data,
2167 size_t len, loff_t pos)
2168 {
2169 struct inode *inode = sb_dqopt(sb)->files[type];
2170 struct address_space *mapping = inode->i_mapping;
2171 size_t toread;
2172 pgoff_t index;
2173 loff_t i_size = i_size_read(inode);
2174
2175 if (pos > i_size)
2176 return 0;
2177 if (pos + len > i_size)
2178 len = i_size - pos;
2179 toread = len;
2180 index = pos / PAGE_SIZE;
2181
2182 while (toread > 0) {
2183 struct folio *folio = read_mapping_folio(mapping, index, NULL);
> 2184 size_t tocopy = min(toread, PAGE_SIZE - offset_in_page(pos));
2185 void *src;
2186
2187 if (folio == ERR_PTR(-ENOMEM)) {
2188 memalloc_retry_wait(GFP_NOFS);
2189 continue;
2190 } else if (IS_ERR(folio))
2191 return PTR_ERR(folio);
2192
2193 src = kmap_local_folio(folio, offset_in_folio(folio, pos));
2194 memcpy(data, src, tocopy);
2195 kunmap_local(src);
2196 folio_put(folio);
2197
2198 toread -= tocopy;
2199 data += tocopy;
2200 pos += tocopy;
2201 index++;
2202 }
2203 return len;
2204 }
2205 EXPORT_SYMBOL(generic_quota_read);
2206
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi "Matthew,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on jack-fs/for_next linus/master v5.19-rc1 next-20220603]
[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]
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/Cache-quota-files-in-the-page-cache/20220606-021629
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: i386-randconfig-s002 (https://download.01.org/0day-ci/archive/20220606/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-18-g56afb504-dirty
# https://github.com/intel-lab-lkp/linux/commit/a7ff347e002ef476c8c116f30858f83529638a9b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/Cache-quota-files-in-the-page-cache/20220606-021629
git checkout a7ff347e002ef476c8c116f30858f83529638a9b
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash fs/quota/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> fs/quota/dquot.c:2184:33: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> fs/quota/dquot.c:2184:33: sparse: unsigned int *
>> fs/quota/dquot.c:2184:33: sparse: unsigned long *
vim +2184 fs/quota/dquot.c
2165
2166 ssize_t generic_quota_read(struct super_block *sb, int type, char *data,
2167 size_t len, loff_t pos)
2168 {
2169 struct inode *inode = sb_dqopt(sb)->files[type];
2170 struct address_space *mapping = inode->i_mapping;
2171 size_t toread;
2172 pgoff_t index;
2173 loff_t i_size = i_size_read(inode);
2174
2175 if (pos > i_size)
2176 return 0;
2177 if (pos + len > i_size)
2178 len = i_size - pos;
2179 toread = len;
2180 index = pos / PAGE_SIZE;
2181
2182 while (toread > 0) {
2183 struct folio *folio = read_mapping_folio(mapping, index, NULL);
> 2184 size_t tocopy = min(toread, PAGE_SIZE - offset_in_page(pos));
2185 void *src;
2186
2187 if (folio == ERR_PTR(-ENOMEM)) {
2188 memalloc_retry_wait(GFP_NOFS);
2189 continue;
2190 } else if (IS_ERR(folio))
2191 return PTR_ERR(folio);
2192
2193 src = kmap_local_folio(folio, offset_in_folio(folio, pos));
2194 memcpy(data, src, tocopy);
2195 kunmap_local(src);
2196 folio_put(folio);
2197
2198 toread -= tocopy;
2199 data += tocopy;
2200 pos += tocopy;
2201 index++;
2202 }
2203 return len;
2204 }
2205 EXPORT_SYMBOL(generic_quota_read);
2206
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Sun 05-06-22 15:38:15, Matthew Wilcox (Oracle) wrote:
> The comment about the page cache is rather stale; the buffer cache will
> read into the page cache if the buffer isn't present, and the page cache
> will not take any locks if the page is present.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
This will not work for couple of reasons, see below. BTW, I don't think the
comment about page cache was stale (but lacking details I admit ;). As far
as I remember (and it was really many years ago - definitely pre-git era)
the problem was (mainly on the write side) that before current state of the
code we were using calls like vfs_read() / vfs_write() to get quota
information and that was indeed prone to deadlocks.
> @@ -6924,20 +6882,21 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> return -EIO;
> }
>
> - do {
> - bh = ext4_bread(handle, inode, blk,
> - EXT4_GET_BLOCKS_CREATE |
> - EXT4_GET_BLOCKS_METADATA_NOFAIL);
> - } while (PTR_ERR(bh) == -ENOSPC &&
> - ext4_should_retry_alloc(inode->i_sb, &retries));
> - if (IS_ERR(bh))
> - return PTR_ERR(bh);
> - if (!bh)
> + folio = read_mapping_folio(inode->i_mapping, off / PAGE_SIZE, NULL);
> + if (IS_ERR(folio))
> + return PTR_ERR(folio);
> + head = folio_buffers(folio);
> + if (!head)
> + head = alloc_page_buffers(&folio->page, sb->s_blocksize, false);
> + if (!head)
> goto out;
> + bh = head;
> + while ((bh_offset(bh) + sb->s_blocksize) <= (off % PAGE_SIZE))
> + bh = bh->b_this_page;
We miss proper handling of blocks that are currently beyond i_size
(we are extending the quota file), plus we also miss any mapping of buffers
to appropriate disk blocks here...
It could be all fixed by replicating what we do in ext4_write_begin() but
I'm not quite convinced using inode's page cache is really worth it...
Honza
> BUFFER_TRACE(bh, "get write access");
> err = ext4_journal_get_write_access(handle, sb, bh, EXT4_JTR_NONE);
> if (err) {
> - brelse(bh);
> + folio_put(folio);
> return err;
> }
> lock_buffer(bh);
> @@ -6945,14 +6904,14 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> flush_dcache_page(bh->b_page);
> unlock_buffer(bh);
> err = ext4_handle_dirty_metadata(handle, NULL, bh);
> - brelse(bh);
> out:
> + folio_put(folio);
> + if (err)
> + return err;
> if (inode->i_size < off + len) {
> i_size_write(inode, off + len);
> EXT4_I(inode)->i_disksize = inode->i_size;
> - err2 = ext4_mark_inode_dirty(handle, inode);
> - if (unlikely(err2 && !err))
> - err = err2;
> + err = ext4_mark_inode_dirty(handle, inode);
> }
> return err ? err : len;
> }
> --
> 2.35.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Jun 06, 2022 at 10:38:14AM +0200, Jan Kara wrote:
> On Sun 05-06-22 15:38:15, Matthew Wilcox (Oracle) wrote:
> > The comment about the page cache is rather stale; the buffer cache will
> > read into the page cache if the buffer isn't present, and the page cache
> > will not take any locks if the page is present.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
>
> This will not work for couple of reasons, see below. BTW, I don't think the
> comment about page cache was stale (but lacking details I admit ;). As far
> as I remember (and it was really many years ago - definitely pre-git era)
> the problem was (mainly on the write side) that before current state of the
> code we were using calls like vfs_read() / vfs_write() to get quota
> information and that was indeed prone to deadlocks.
Ah yes, vfs_write() might indeed be prone to deadlocks. Particularly
if we're doing it under the dq_mutex and any memory allocation might
have recursed into reclaim ;-)
I actually found the commit in linux-fullhistory. Changelog for
context:
commit b72debd66a6ed
Author: Jan Kara <[email protected]>
Date: Mon Jan 3 04:12:24 2005 -0800
[PATCH] Fix of quota deadlock on pagelock: quota core
The four patches in this series fix deadlocks with quotas of pagelock (the
problem was lock inversion on PageLock and transaction start - quota code
needed to first start a transaction and then write the data which subsequent
ly
needed acquisition of PageLock while the standard ordering - PageLock first
and transaction start later - was used e.g. by pdflush). They implement a
new way of quota access to disk: Every filesystem that would like to impleme
nt
quotas now has to provide quota_read() and quota_write() functions. These
functions must obey quota lock ordering (in particular they should not take
PageLock inside a transaction).
The first patch implements the changes in the quota core, the other three
patches implement needed functions in ext2, ext3 and reiserfs. The patch for
reiserfs also fixes several other lock inversion problems (similar as ext3
had) and implements the journaled quota functionality (which comes almost for
free after the locking fixes...).
The quota core patch makes quota support in other filesystems (except XFS
which implements everything on its own ;)) unfunctional (quotaon() will refuse
to turn on quotas on them). When the patches get reasonable wide testing and
it will seem that no major changes will be needed I can make fixes also for
the other filesystems (JFS, UDF, UFS).
This patch:
The patch implements the new way of quota io in the quota core. Every
filesystem wanting to support quotas has to provide functions quota_read()
and quota_write() obeying quota locking rules. As the writes and reads
bypass the pagecache there is some ugly stuff ensuring that userspace can
see all the data after quotaoff() (or Q_SYNC quotactl). In future I plan
to make quota files inaccessible from userspace (with the exception of
quotacheck(8) which will take care about the cache flushing and such stuff
itself) so that this synchronization stuff can be removed...
The rewrite of the quota core. Quota uses the filesystem read() and write()
functions no more to avoid possible deadlocks on PageLock. From now on every
filesystem supporting quotas must provide functions quota_read() and
quota_write() which obey the quota locking rules (e.g. they cannot acquire the
PageLock).
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
> > @@ -6924,20 +6882,21 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> > return -EIO;
> > }
> >
> > - do {
> > - bh = ext4_bread(handle, inode, blk,
> > - EXT4_GET_BLOCKS_CREATE |
> > - EXT4_GET_BLOCKS_METADATA_NOFAIL);
> > - } while (PTR_ERR(bh) == -ENOSPC &&
> > - ext4_should_retry_alloc(inode->i_sb, &retries));
> > - if (IS_ERR(bh))
> > - return PTR_ERR(bh);
> > - if (!bh)
> > + folio = read_mapping_folio(inode->i_mapping, off / PAGE_SIZE, NULL);
> > + if (IS_ERR(folio))
> > + return PTR_ERR(folio);
> > + head = folio_buffers(folio);
> > + if (!head)
> > + head = alloc_page_buffers(&folio->page, sb->s_blocksize, false);
> > + if (!head)
> > goto out;
> > + bh = head;
> > + while ((bh_offset(bh) + sb->s_blocksize) <= (off % PAGE_SIZE))
> > + bh = bh->b_this_page;
>
> We miss proper handling of blocks that are currently beyond i_size
> (we are extending the quota file), plus we also miss any mapping of buffers
> to appropriate disk blocks here...
>
> It could be all fixed by replicating what we do in ext4_write_begin() but
> I'm not quite convinced using inode's page cache is really worth it...
Ah, yes, write_begin. Of course that's what I should have used.
I'm looking at this from the point of view of removing buffer_heads
where possible. Of course, it's not possible for ext4 while the journal
relies on buffer_heads, but if we can steer filesystems away from using
sb_bread() (or equivalents), I think that's a good thing.
On Wed 08-06-22 02:42:21, Matthew Wilcox wrote:
> On Mon, Jun 06, 2022 at 10:38:14AM +0200, Jan Kara wrote:
> > On Sun 05-06-22 15:38:15, Matthew Wilcox (Oracle) wrote:
> > > The comment about the page cache is rather stale; the buffer cache will
> > > read into the page cache if the buffer isn't present, and the page cache
> > > will not take any locks if the page is present.
> > >
> > > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> >
> > This will not work for couple of reasons, see below. BTW, I don't think the
> > comment about page cache was stale (but lacking details I admit ;). As far
> > as I remember (and it was really many years ago - definitely pre-git era)
> > the problem was (mainly on the write side) that before current state of the
> > code we were using calls like vfs_read() / vfs_write() to get quota
> > information and that was indeed prone to deadlocks.
>
> Ah yes, vfs_write() might indeed be prone to deadlocks. Particularly
> if we're doing it under the dq_mutex and any memory allocation might
> have recursed into reclaim ;-)
>
> I actually found the commit in linux-fullhistory. Changelog for
> context:
>
> commit b72debd66a6ed
> Author: Jan Kara <[email protected]>
> Date: Mon Jan 3 04:12:24 2005 -0800
>
> [PATCH] Fix of quota deadlock on pagelock: quota core
\o/ to you history searching skills :)
> > > @@ -6924,20 +6882,21 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
> > > return -EIO;
> > > }
> > >
> > > - do {
> > > - bh = ext4_bread(handle, inode, blk,
> > > - EXT4_GET_BLOCKS_CREATE |
> > > - EXT4_GET_BLOCKS_METADATA_NOFAIL);
> > > - } while (PTR_ERR(bh) == -ENOSPC &&
> > > - ext4_should_retry_alloc(inode->i_sb, &retries));
> > > - if (IS_ERR(bh))
> > > - return PTR_ERR(bh);
> > > - if (!bh)
> > > + folio = read_mapping_folio(inode->i_mapping, off / PAGE_SIZE, NULL);
> > > + if (IS_ERR(folio))
> > > + return PTR_ERR(folio);
> > > + head = folio_buffers(folio);
> > > + if (!head)
> > > + head = alloc_page_buffers(&folio->page, sb->s_blocksize, false);
> > > + if (!head)
> > > goto out;
> > > + bh = head;
> > > + while ((bh_offset(bh) + sb->s_blocksize) <= (off % PAGE_SIZE))
> > > + bh = bh->b_this_page;
> >
> > We miss proper handling of blocks that are currently beyond i_size
> > (we are extending the quota file), plus we also miss any mapping of buffers
> > to appropriate disk blocks here...
> >
> > It could be all fixed by replicating what we do in ext4_write_begin() but
> > I'm not quite convinced using inode's page cache is really worth it...
>
> Ah, yes, write_begin. Of course that's what I should have used.
>
> I'm looking at this from the point of view of removing buffer_heads
> where possible. Of course, it's not possible for ext4 while the journal
> relies on buffer_heads, but if we can steer filesystems away from using
> sb_bread() (or equivalents), I think that's a good thing.
Well, ext4 uses sb_bread() (sb_getblk()) for all its metadata so quota
code, which is rather well localized, is the least of your worries I'm
afraid ;).
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: fa964903692268d3913cdaf489f80db849a3e928 ("[PATCH 3/3] ext4: Use generic_quota_read()")
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/Cache-quota-files-in-the-page-cache/20220606-021629
base: https://git.kernel.org/cgit/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/linux-ext4/[email protected]
in testcase: ltp
version: ltp-x86_64-14c1f76-1_20220604
with following parameters:
disk: 1HDD
fs: btrfs
test: fs-03
ucode: 0xec
test-description: The LTP testsuite contains a collection of tools for testing the Linux kernel and related features.
test-url: http://linux-test-project.github.io/
on test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 366.801777][ T5247] WARNING: CPU: 5 PID: 5247 at fs/ext4/inode.c:3228 ext4_invalidate_folio (fs/ext4/inode.c:3228)
[ 366.811124][ T5247] Modules linked in: loop binfmt_misc dm_mod btrfs blake2b_generic xor raid6_pq intel_rapl_msr intel_rapl_common zstd_compress libcrc32c i915 sd_mod t10_pi crc64_rocksoft_generic crc64_rocksoft crc64 sg x86_pkg_temp_thermal intel_gtt drm_buddy intel_powerclamp ipmi_devintf ipmi_msghandler coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel drm_dp_helper wmi_bmof mei_wdt ttm rapl intel_cstate drm_kms_helper ahci libahci syscopyarea sysfillrect i2c_designware_platform intel_uncore mei_me libata sysimgblt i2c_designware_core mei idma64 fb_sys_fops wmi video intel_pmc_core acpi_pad drm fuse ip_tables
[ 366.869509][ T5247] CPU: 5 PID: 5247 Comm: umount Tainted: G I 5.18.0-rc5-00028-gfa9649036922 #1
[ 366.879632][ T5247] Hardware name: Dell Inc. OptiPlex 7050/062KRH, BIOS 1.2.0 12/22/2016
[ 366.887739][ T5247] RIP: 0010:ext4_invalidate_folio (fs/ext4/inode.c:3228)
[ 366.893597][ T5247] Code: e8 f5 12 c7 ff 48 89 da 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 80 3c 02 00 0f 85 c1 00 00 00 48 8b 03 a9 00 00 01 00 74 02 <0f> 0b 5b 4c 89 ea 4c 89 e6 48 89 ef 5d 41 5c 41 5d e9 3a 4a df ff
All code
========
0: e8 f5 12 c7 ff callq 0xffffffffffc712fa
5: 48 89 da mov %rbx,%rdx
8: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
f: fc ff df
12: 48 c1 ea 03 shr $0x3,%rdx
16: 80 3c 02 00 cmpb $0x0,(%rdx,%rax,1)
1a: 0f 85 c1 00 00 00 jne 0xe1
20: 48 8b 03 mov (%rbx),%rax
23: a9 00 00 01 00 test $0x10000,%eax
28: 74 02 je 0x2c
2a:* 0f 0b ud2 <-- trapping instruction
2c: 5b pop %rbx
2d: 4c 89 ea mov %r13,%rdx
30: 4c 89 e6 mov %r12,%rsi
33: 48 89 ef mov %rbp,%rdi
36: 5d pop %rbp
37: 41 5c pop %r12
39: 41 5d pop %r13
3b: e9 3a 4a df ff jmpq 0xffffffffffdf4a7a
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: 5b pop %rbx
3: 4c 89 ea mov %r13,%rdx
6: 4c 89 e6 mov %r12,%rsi
9: 48 89 ef mov %rbp,%rdi
c: 5d pop %rbp
d: 41 5c pop %r12
f: 41 5d pop %r13
11: e9 3a 4a df ff jmpq 0xffffffffffdf4a50
[ 366.913124][ T5247] RSP: 0018:ffffc90000e3fab0 EFLAGS: 00010206
[ 366.919089][ T5247] RAX: 000000000011601b RBX: ffff8887f2c18dc8 RCX: ffffffff81c7828b
[ 366.926940][ T5247] RDX: 1ffff110fe5831b9 RSI: 0000000000000008 RDI: ffff8887f2c18dc8
[ 366.934793][ T5247] RBP: ffffea00072cd700 R08: 0000000000000000 R09: ffff8887f2c18dcf
[ 366.942641][ T5247] R10: ffffed10fe5831b9 R11: 0000000000000001 R12: 0000000000000000
[ 366.950492][ T5247] R13: 0000000000001000 R14: 0000000000000002 R15: 0000000000000001
[ 366.958343][ T5247] FS: 00007f5852a94840(0000) GS:ffff8886aaa80000(0000) knlGS:0000000000000000
[ 366.967151][ T5247] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 366.973607][ T5247] CR2: 0000561684be9559 CR3: 00000007f60fc006 CR4: 00000000003706e0
[ 366.981457][ T5247] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 366.989303][ T5247] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 366.997156][ T5247] Call Trace:
[ 367.000322][ T5247] <TASK>
[ 367.003130][ T5247] truncate_cleanup_folio (mm/truncate.c:159 mm/truncate.c:179)
[ 367.008372][ T5247] truncate_inode_pages_range (include/linux/pagevec.h:119 (discriminator 3) mm/truncate.c:368 (discriminator 3))
[ 367.013965][ T5247] ? truncate_inode_partial_folio (mm/truncate.c:332)
[ 367.019900][ T5247] ? jbd2_journal_set_features (fs/jbd2/journal.c:682)
[ 367.025580][ T5247] ? _raw_write_lock (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qrwlock.h:94 include/linux/rwlock_api_smp.h:210 kernel/locking/spinlock.c:300)
[ 367.030300][ T5247] ? prepare_to_swait_exclusive (kernel/sched/wait.c:414)
[ 367.036082][ T5247] ? inode_to_bdi (mm/backing-dev.c:995 mm/backing-dev.c:985)
[ 367.040540][ T5247] ? filemap_check_errors (arch/x86/include/asm/bitops.h:207 include/asm-generic/bitops/instrumented-non-atomic.h:135 mm/filemap.c:351)
[ 367.045712][ T5247] ? watch_queue_set_size (kernel/watch_queue.c:219)
[ 367.050864][ T5247] ? filemap_fdatawait_keep_errors (mm/filemap.c:669)
[ 367.056715][ T5247] ? __cond_resched (kernel/sched/core.c:8177)
[ 367.061262][ T5247] ? down_write (arch/x86/include/asm/atomic64_64.h:34 include/linux/atomic/atomic-long.h:41 include/linux/atomic/atomic-instrumented.h:1280 kernel/locking/rwsem.c:138 kernel/locking/rwsem.c:255 kernel/locking/rwsem.c:1258 kernel/locking/rwsem.c:1268 kernel/locking/rwsem.c:1515)
[ 367.065562][ T5247] ? down_write_killable (kernel/locking/rwsem.c:1512)
[ 367.070715][ T5247] ? v2_free_file_info (fs/quota/quota_v2.c:384)
[ 367.075530][ T5247] ? kfree (mm/slub.c:1754 mm/slub.c:3510 mm/slub.c:4552)
[ 367.079385][ T5247] dquot_disable (fs/quota/dquot.c:2355)
[ 367.083850][ T5247] ext4_quota_off (fs/ext4/super.c:6831)
[ 367.088314][ T5247] ext4_put_super (fs/ext4/super.c:1173 fs/ext4/super.c:1218)
[ 367.092779][ T5247] generic_shutdown_super (fs/super.c:464)
[ 367.098029][ T5247] kill_block_super (fs/super.c:1395)
[ 367.102680][ T5247] deactivate_locked_super (fs/super.c:339)
[ 367.107923][ T5247] cleanup_mnt (fs/namespace.c:138 fs/namespace.c:1187)
[ 367.112210][ T5247] ? path_umount (fs/namespace.c:1808)
[ 367.116682][ T5247] task_work_run (kernel/task_work.c:166 (discriminator 1))
[ 367.121091][ T5247] exit_to_user_mode_loop (include/linux/resume_user_mode.h:49 kernel/entry/common.c:169)
[ 367.126338][ T5247] exit_to_user_mode_prepare (kernel/entry/common.c:201)
[ 367.131756][ T5247] syscall_exit_to_user_mode (arch/x86/include/asm/jump_label.h:27 include/linux/context_tracking_state.h:31 include/linux/context_tracking.h:40 kernel/entry/common.c:132 kernel/entry/common.c:296)
[ 367.137112][ T5247] do_syscall_64 (arch/x86/entry/common.c:87)
[ 367.141398][ T5247] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
[ 367.147174][ T5247] RIP: 0033:0x7f5852cd3e27
[ 367.151460][ T5247] Code: 00 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09 00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 39 00 0c 00 f7 d8 64 89 01 48
All code
========
0: 00 0c 00 add %cl,(%rax,%rax,1)
3: f7 d8 neg %eax
5: 64 89 01 mov %eax,%fs:(%rcx)
8: 48 83 c8 ff or $0xffffffffffffffff,%rax
c: c3 retq
d: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
13: 31 f6 xor %esi,%esi
15: e9 09 00 00 00 jmpq 0x23
1a: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
21: 00 00
23: b8 a6 00 00 00 mov $0xa6,%eax
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d 39 00 0c 00 mov 0xc0039(%rip),%rcx # 0xc0073
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d 39 00 0c 00 mov 0xc0039(%rip),%rcx # 0xc0049
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://01.org/lkp