From b846f2d7e2d28ebfa112ac595bde2ef87693d8d9 Mon Sep 17 00:00:00 2001 From: Jakob Koschel Date: Fri, 1 Apr 2022 00:38:57 +0200 Subject: gfs2: replace 'found' with dedicated list iterator variable To move the list iterator variable into the list_for_each_entry_*() macro in the future it should be avoided to use the list iterator variable after the loop body. To *never* use the list iterator variable after the loop it was concluded to use a separate iterator variable instead of a found boolean [1]. This removes the need to use a found variable and simply checking if the variable was set, can determine if the break/goto was hit. Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] Signed-off-by: Jakob Koschel Signed-off-by: Andreas Gruenbacher --- fs/gfs2/quota.c | 13 +++++-------- fs/gfs2/recovery.c | 22 ++++++++++------------ 2 files changed, 15 insertions(+), 20 deletions(-) (limited to 'fs/gfs2') diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c index be0997e24d60..ec7e0910b278 100644 --- a/fs/gfs2/quota.c +++ b/fs/gfs2/quota.c @@ -443,9 +443,8 @@ static int qd_check_sync(struct gfs2_sbd *sdp, struct gfs2_quota_data *qd, static int qd_fish(struct gfs2_sbd *sdp, struct gfs2_quota_data **qdp) { - struct gfs2_quota_data *qd = NULL; + struct gfs2_quota_data *qd = NULL, *iter; int error; - int found = 0; *qdp = NULL; @@ -454,15 +453,13 @@ static int qd_fish(struct gfs2_sbd *sdp, struct gfs2_quota_data **qdp) spin_lock(&qd_lock); - list_for_each_entry(qd, &sdp->sd_quota_list, qd_list) { - found = qd_check_sync(sdp, qd, &sdp->sd_quota_sync_gen); - if (found) + list_for_each_entry(iter, &sdp->sd_quota_list, qd_list) { + if (qd_check_sync(sdp, iter, &sdp->sd_quota_sync_gen)) { + qd = iter; break; + } } - if (!found) - qd = NULL; - spin_unlock(&qd_lock); if (qd) { diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c index 016ed1b2ca1d..2bb085a72e8e 100644 --- a/fs/gfs2/recovery.c +++ b/fs/gfs2/recovery.c @@ -55,17 +55,16 @@ int gfs2_replay_read_block(struct gfs2_jdesc *jd, unsigned int blk, int gfs2_revoke_add(struct gfs2_jdesc *jd, u64 blkno, unsigned int where) { struct list_head *head = &jd->jd_revoke_list; - struct gfs2_revoke_replay *rr; - int found = 0; + struct gfs2_revoke_replay *rr = NULL, *iter; - list_for_each_entry(rr, head, rr_list) { - if (rr->rr_blkno == blkno) { - found = 1; + list_for_each_entry(iter, head, rr_list) { + if (iter->rr_blkno == blkno) { + rr = iter; break; } } - if (found) { + if (rr) { rr->rr_where = where; return 0; } @@ -83,18 +82,17 @@ int gfs2_revoke_add(struct gfs2_jdesc *jd, u64 blkno, unsigned int where) int gfs2_revoke_check(struct gfs2_jdesc *jd, u64 blkno, unsigned int where) { - struct gfs2_revoke_replay *rr; + struct gfs2_revoke_replay *rr = NULL, *iter; int wrap, a, b, revoke; - int found = 0; - list_for_each_entry(rr, &jd->jd_revoke_list, rr_list) { - if (rr->rr_blkno == blkno) { - found = 1; + list_for_each_entry(iter, &jd->jd_revoke_list, rr_list) { + if (iter->rr_blkno == blkno) { + rr = iter; break; } } - if (!found) + if (!rr) return 0; wrap = (rr->rr_where < jd->jd_replay_tail); -- cgit From 53bb540fd591f6fdd4cb5c9a785d9790ac33862d Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Tue, 26 Apr 2022 23:31:06 +0200 Subject: gfs2: Explain some direct I/O oddities Add some comments explaining the oddities of partial direct I/O reads and writes. Signed-off-by: Andreas Gruenbacher --- fs/gfs2/file.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'fs/gfs2') diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 2556ae1f92ea..f1d2f4d74b89 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -840,6 +840,7 @@ retry: pagefault_enable(); if (ret <= 0 && ret != -EFAULT) goto out_unlock; + /* No increment (+=) because iomap_dio_rw returns a cumulative value. */ if (ret > 0) read = ret; @@ -854,6 +855,7 @@ out_unlock: gfs2_glock_dq(gh); out_uninit: gfs2_holder_uninit(gh); + /* User space doesn't expect partial success. */ if (ret < 0) return ret; return read; @@ -906,6 +908,7 @@ retry: if (ret != -EFAULT) goto out_unlock; } + /* No increment (+=) because iomap_dio_rw returns a cumulative value. */ if (ret > 0) written = ret; @@ -920,6 +923,7 @@ out_unlock: gfs2_glock_dq(gh); out_uninit: gfs2_holder_uninit(gh); + /* User space doesn't expect partial success. */ if (ret < 0) return ret; return written; -- cgit From 11d8b79e849db099b04584913880a799549aaad5 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Sun, 8 May 2022 03:06:30 -0700 Subject: gfs2: Use container_of() for gfs2_glock(aspace) Clang's structure layout randomization feature gets upset when it sees struct address_space (which is randomized) cast to struct gfs2_glock. This is due to seeing the mapping pointer as being treated as an array of gfs2_glock, rather than "something else, before struct address_space": In file included from fs/gfs2/acl.c:23: fs/gfs2/meta_io.h:44:12: error: casting from randomized structure pointer type 'struct address_space *' to 'struct gfs2_glock *' return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd; ^ Replace the instances of open-coded pointer math with container_of() usage, and update the allocator to match. Some cleanups and conversion of gfs2_glock_get() and gfs2_glock_dealloc() by Andreas. Reported-by: kernel test robot Link: https://lore.kernel.org/lkml/202205041550.naKxwCBj-lkp@intel.com Cc: Bob Peterson Cc: Andreas Gruenbacher Cc: Bill Wendling Cc: cluster-devel@redhat.com Signed-off-by: Kees Cook Signed-off-by: Andreas Gruenbacher --- fs/gfs2/glock.c | 35 +++++++++++++++++++---------------- fs/gfs2/glock.h | 12 ++++++++++-- fs/gfs2/main.c | 10 ++++------ fs/gfs2/meta_io.h | 8 +++++--- 4 files changed, 38 insertions(+), 27 deletions(-) (limited to 'fs/gfs2') diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 630c6550eacf..c992d53013d3 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -127,9 +127,11 @@ static void gfs2_glock_dealloc(struct rcu_head *rcu) struct gfs2_glock *gl = container_of(rcu, struct gfs2_glock, gl_rcu); kfree(gl->gl_lksb.sb_lvbptr); - if (gl->gl_ops->go_flags & GLOF_ASPACE) - kmem_cache_free(gfs2_glock_aspace_cachep, gl); - else + if (gl->gl_ops->go_flags & GLOF_ASPACE) { + struct gfs2_glock_aspace *gla = + container_of(gl, struct gfs2_glock_aspace, glock); + kmem_cache_free(gfs2_glock_aspace_cachep, gla); + } else kmem_cache_free(gfs2_glock_cachep, gl); } @@ -1159,7 +1161,6 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number, .ln_sbd = sdp }; struct gfs2_glock *gl, *tmp; struct address_space *mapping; - struct kmem_cache *cachep; int ret = 0; gl = find_insert_glock(&name, NULL); @@ -1170,20 +1171,24 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number, if (!create) return -ENOENT; - if (glops->go_flags & GLOF_ASPACE) - cachep = gfs2_glock_aspace_cachep; - else - cachep = gfs2_glock_cachep; - gl = kmem_cache_alloc(cachep, GFP_NOFS); - if (!gl) - return -ENOMEM; - + if (glops->go_flags & GLOF_ASPACE) { + struct gfs2_glock_aspace *gla = + kmem_cache_alloc(gfs2_glock_aspace_cachep, GFP_NOFS); + if (!gla) + return -ENOMEM; + gl = &gla->glock; + } else { + gl = kmem_cache_alloc(gfs2_glock_cachep, GFP_NOFS); + if (!gl) + return -ENOMEM; + } memset(&gl->gl_lksb, 0, sizeof(struct dlm_lksb)); + gl->gl_ops = glops; if (glops->go_flags & GLOF_LVB) { gl->gl_lksb.sb_lvbptr = kzalloc(GDLM_LVB_SIZE, GFP_NOFS); if (!gl->gl_lksb.sb_lvbptr) { - kmem_cache_free(cachep, gl); + gfs2_glock_dealloc(&gl->gl_rcu); return -ENOMEM; } } @@ -1197,7 +1202,6 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number, gl->gl_state = LM_ST_UNLOCKED; gl->gl_target = LM_ST_UNLOCKED; gl->gl_demote_state = LM_ST_EXCLUSIVE; - gl->gl_ops = glops; gl->gl_dstamp = 0; preempt_disable(); /* We use the global stats to estimate the initial per-glock stats */ @@ -1234,8 +1238,7 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number, *glp = tmp; out_free: - kfree(gl->gl_lksb.sb_lvbptr); - kmem_cache_free(cachep, gl); + gfs2_glock_dealloc(&gl->gl_rcu); if (atomic_dec_and_test(&sdp->sd_glock_disposal)) wake_up(&sdp->sd_glock_wait); diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h index 4f8642301801..c0ae9100a0bc 100644 --- a/fs/gfs2/glock.h +++ b/fs/gfs2/glock.h @@ -138,6 +138,11 @@ struct lm_lockops { const match_table_t *lm_tokens; }; +struct gfs2_glock_aspace { + struct gfs2_glock glock; + struct address_space mapping; +}; + extern struct workqueue_struct *gfs2_delete_workqueue; static inline struct gfs2_holder *gfs2_glock_is_locked_by_me(struct gfs2_glock *gl) { @@ -179,8 +184,11 @@ static inline int gfs2_glock_is_held_shrd(struct gfs2_glock *gl) static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl) { - if (gl->gl_ops->go_flags & GLOF_ASPACE) - return (struct address_space *)(gl + 1); + if (gl->gl_ops->go_flags & GLOF_ASPACE) { + struct gfs2_glock_aspace *gla = + container_of(gl, struct gfs2_glock_aspace, glock); + return &gla->mapping; + } return NULL; } diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c index 28d0eb23e18e..244187e3e70f 100644 --- a/fs/gfs2/main.c +++ b/fs/gfs2/main.c @@ -62,11 +62,10 @@ static void gfs2_init_glock_once(void *foo) static void gfs2_init_gl_aspace_once(void *foo) { - struct gfs2_glock *gl = foo; - struct address_space *mapping = (struct address_space *)(gl + 1); + struct gfs2_glock_aspace *gla = foo; - gfs2_init_glock_once(gl); - address_space_init_once(mapping); + gfs2_init_glock_once(&gla->glock); + address_space_init_once(&gla->mapping); } /** @@ -104,8 +103,7 @@ static int __init init_gfs2_fs(void) goto fail_cachep1; gfs2_glock_aspace_cachep = kmem_cache_create("gfs2_glock(aspace)", - sizeof(struct gfs2_glock) + - sizeof(struct address_space), + sizeof(struct gfs2_glock_aspace), 0, 0, gfs2_init_gl_aspace_once); if (!gfs2_glock_aspace_cachep) diff --git a/fs/gfs2/meta_io.h b/fs/gfs2/meta_io.h index 21880d72081a..d0a58cdd433a 100644 --- a/fs/gfs2/meta_io.h +++ b/fs/gfs2/meta_io.h @@ -40,9 +40,11 @@ extern const struct address_space_operations gfs2_rgrp_aops; static inline struct gfs2_sbd *gfs2_mapping2sbd(struct address_space *mapping) { struct inode *inode = mapping->host; - if (mapping->a_ops == &gfs2_meta_aops) - return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd; - else if (mapping->a_ops == &gfs2_rgrp_aops) + if (mapping->a_ops == &gfs2_meta_aops) { + struct gfs2_glock_aspace *gla = + container_of(mapping, struct gfs2_glock_aspace, mapping); + return gla->glock.gl_name.ln_sbd; + } else if (mapping->a_ops == &gfs2_rgrp_aops) return container_of(mapping, struct gfs2_sbd, sd_aspace); else return inode->i_sb->s_fs_info; -- cgit From f4a47561fcc1494ee3f273163189cf4462b6a245 Mon Sep 17 00:00:00 2001 From: Andrew Price Date: Tue, 22 Mar 2022 18:49:19 +0000 Subject: gfs2: Return more useful errors from gfs2_rgrp_send_discards() The bug that 27ca8273f ("gfs2: Make sure FITRIM minlen is rounded up to fs block size") fixes was a little confusing as the user saw "Input/output error" which masked the -EINVAL that sb_issue_discard() returned. sb_issue_discard() can fail for various reasons, so we should return its return value from gfs2_rgrp_send_discards() to avoid all errors being reported as IO errors. This improves error reporting for FITRIM and makes no difference to the -o discard code path because the return value from gfs2_rgrp_send_discards() gets thrown away in that case (and the option switches off). Presumably that's why it was ok to just return -EIO in the past, before FITRIM was implemented. Tested with xfstests. Signed-off-by: Andrew Price Signed-off-by: Andreas Gruenbacher --- fs/gfs2/rgrp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/gfs2') diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c index 801ad9f4f2be..886343cc05ab 100644 --- a/fs/gfs2/rgrp.c +++ b/fs/gfs2/rgrp.c @@ -1315,7 +1315,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset, u64 blk; sector_t start = 0; sector_t nr_blks = 0; - int rv; + int rv = -EIO; unsigned int x; u32 trimmed = 0; u8 diff; @@ -1371,7 +1371,7 @@ fail: if (sdp->sd_args.ar_discard) fs_warn(sdp, "error %d on discard request, turning discards off for this filesystem\n", rv); sdp->sd_args.ar_discard = 0; - return -EIO; + return rv; } /** -- cgit From 5fcff61eea9efd1f4b60e89d2d686b5feaea100f Mon Sep 17 00:00:00 2001 From: Bob Peterson Date: Fri, 11 Feb 2022 10:50:08 -0500 Subject: gfs2: use i_lock spin_lock for inode qadata Before this patch, functions gfs2_qa_get and _put used the i_rw_mutex to prevent simultaneous access to its i_qadata. But i_rw_mutex is now used for many other things, including iomap_begin and end, which causes a conflict according to lockdep. We cannot just remove the lock since simultaneous opens (gfs2_open -> gfs2_open_common -> gfs2_qa_get) can then stomp on each others values for i_qadata. This patch solves the conflict by using the i_lock spin_lock in the inode to prevent simultaneous access. Signed-off-by: Bob Peterson Signed-off-by: Andreas Gruenbacher --- fs/gfs2/quota.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) (limited to 'fs/gfs2') diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c index ec7e0910b278..390f34ca3bc8 100644 --- a/fs/gfs2/quota.c +++ b/fs/gfs2/quota.c @@ -528,34 +528,42 @@ static void qdsb_put(struct gfs2_quota_data *qd) */ int gfs2_qa_get(struct gfs2_inode *ip) { - int error = 0; struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode); + struct inode *inode = &ip->i_inode; if (sdp->sd_args.ar_quota == GFS2_QUOTA_OFF) return 0; - down_write(&ip->i_rw_mutex); + spin_lock(&inode->i_lock); if (ip->i_qadata == NULL) { - ip->i_qadata = kmem_cache_zalloc(gfs2_qadata_cachep, GFP_NOFS); - if (!ip->i_qadata) { - error = -ENOMEM; - goto out; - } + struct gfs2_qadata *tmp; + + spin_unlock(&inode->i_lock); + tmp = kmem_cache_zalloc(gfs2_qadata_cachep, GFP_NOFS); + if (!tmp) + return -ENOMEM; + + spin_lock(&inode->i_lock); + if (ip->i_qadata == NULL) + ip->i_qadata = tmp; + else + kmem_cache_free(gfs2_qadata_cachep, tmp); } ip->i_qadata->qa_ref++; -out: - up_write(&ip->i_rw_mutex); - return error; + spin_unlock(&inode->i_lock); + return 0; } void gfs2_qa_put(struct gfs2_inode *ip) { - down_write(&ip->i_rw_mutex); + struct inode *inode = &ip->i_inode; + + spin_lock(&inode->i_lock); if (ip->i_qadata && --ip->i_qadata->qa_ref == 0) { kmem_cache_free(gfs2_qadata_cachep, ip->i_qadata); ip->i_qadata = NULL; } - up_write(&ip->i_rw_mutex); + spin_unlock(&inode->i_lock); } int gfs2_quota_hold(struct gfs2_inode *ip, kuid_t uid, kgid_t gid) -- cgit From c360abbb9db298d0548b31e1a86a48ebb157d7cd Mon Sep 17 00:00:00 2001 From: Bob Peterson Date: Fri, 11 Feb 2022 10:50:36 -0500 Subject: gfs2: Convert function bh_get to use iomap Before this patch, function bh_get used block_map to figure out the block it needed to read in from the quota_change file. This patch changes it to use iomap directly to make it more efficient. Signed-off-by: Bob Peterson Signed-off-by: Andreas Gruenbacher --- fs/gfs2/quota.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) (limited to 'fs/gfs2') diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c index 390f34ca3bc8..59d727a4ae2c 100644 --- a/fs/gfs2/quota.c +++ b/fs/gfs2/quota.c @@ -365,11 +365,12 @@ static void slot_put(struct gfs2_quota_data *qd) static int bh_get(struct gfs2_quota_data *qd) { struct gfs2_sbd *sdp = qd->qd_gl->gl_name.ln_sbd; - struct gfs2_inode *ip = GFS2_I(sdp->sd_qc_inode); + struct inode *inode = sdp->sd_qc_inode; + struct gfs2_inode *ip = GFS2_I(inode); unsigned int block, offset; struct buffer_head *bh; + struct iomap iomap = { }; int error; - struct buffer_head bh_map = { .b_state = 0, .b_blocknr = 0 }; mutex_lock(&sdp->sd_quota_mutex); @@ -381,11 +382,17 @@ static int bh_get(struct gfs2_quota_data *qd) block = qd->qd_slot / sdp->sd_qc_per_block; offset = qd->qd_slot % sdp->sd_qc_per_block; - bh_map.b_size = BIT(ip->i_inode.i_blkbits); - error = gfs2_block_map(&ip->i_inode, block, &bh_map, 0); + error = gfs2_iomap_get(inode, + (loff_t)block << inode->i_blkbits, + i_blocksize(inode), &iomap); if (error) goto fail; - error = gfs2_meta_read(ip->i_gl, bh_map.b_blocknr, DIO_WAIT, 0, &bh); + error = -ENOENT; + if (iomap.type != IOMAP_MAPPED) + goto fail; + + error = gfs2_meta_read(ip->i_gl, iomap.addr >> inode->i_blkbits, + DIO_WAIT, 0, &bh); if (error) goto fail; error = -EIO; -- cgit