From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bob Peterson Date: Thu, 26 Mar 2020 13:36:01 -0500 Subject: [Cluster-devel] [GFS2 PATCH 2/4] gfs2: Change inode qa_data to allow multiple users In-Reply-To: <20200326183603.123323-1-rpeterso@redhat.com> References: <20200326183603.123323-1-rpeterso@redhat.com> Message-ID: <20200326183603.123323-3-rpeterso@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Before this patch, multiple users called gfs2_qa_alloc which allocated a qadata structure to the inode, if quotas are turned on. Later, in file close or evict, the structure was deleted with gfs2_qa_delete. But there can be several competing processes who need access to the structure. There were races between file close (release) and the others. Thus, a release could delete the structure out from under a process that relied upon its existence. For example, chown. This patch changes the management of the qadata structures to be a get/put scheme. Function gfs2_qa_alloc has been changed to gfs2_qa_get and if the structure is allocated, the count essentially starts out at 1. Function gfs2_qa_delete has been renamed to gfs2_qa_put, and the last guy to decrement the count to 0 frees the memory. Signed-off-by: Bob Peterson --- fs/gfs2/acl.c | 6 +++-- fs/gfs2/bmap.c | 2 +- fs/gfs2/file.c | 35 +++++++++++++++++++-------- fs/gfs2/incore.h | 1 + fs/gfs2/inode.c | 32 ++++++++++++++---------- fs/gfs2/quota.c | 63 +++++++++++++++++++++++++++++------------------- fs/gfs2/quota.h | 4 +-- fs/gfs2/rgrp.c | 2 +- fs/gfs2/super.c | 2 ++ fs/gfs2/xattr.c | 12 ++++++--- 10 files changed, 101 insertions(+), 58 deletions(-) diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c index cb09b85c5b10..2e939f5fe751 100644 --- a/fs/gfs2/acl.c +++ b/fs/gfs2/acl.c @@ -117,14 +117,14 @@ int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type) if (acl && acl->a_count > GFS2_ACL_MAX_ENTRIES(GFS2_SB(inode))) return -E2BIG; - ret = gfs2_qa_alloc(ip); + ret = gfs2_qa_get(ip); if (ret) return ret; if (!gfs2_glock_is_locked_by_me(ip->i_gl)) { ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh); if (ret) - return ret; + goto out; need_unlock = true; } @@ -144,5 +144,7 @@ int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type) unlock: if (need_unlock) gfs2_glock_dq_uninit(&gh); +out: + gfs2_qa_put(ip); return ret; } diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c index 0f32d2ceb0af..bb67d2ee1b40 100644 --- a/fs/gfs2/bmap.c +++ b/fs/gfs2/bmap.c @@ -2183,7 +2183,7 @@ int gfs2_setattr_size(struct inode *inode, u64 newsize) inode_dio_wait(inode); - ret = gfs2_qa_alloc(ip); + ret = gfs2_qa_get(ip); if (ret) goto out; diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 54b0708e6d35..f18876cdfb0f 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -458,7 +458,7 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf) sb_start_pagefault(inode->i_sb); - ret = gfs2_qa_alloc(ip); + ret = gfs2_qa_get(ip); if (ret) goto out; @@ -553,6 +553,7 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf) out_unlock: gfs2_glock_dq(&gh); out_uninit: + gfs2_qa_put(ip); gfs2_holder_uninit(&gh); if (ret == 0) { set_page_dirty(page); @@ -635,7 +636,17 @@ int gfs2_open_common(struct inode *inode, struct file *file) gfs2_assert_warn(GFS2_SB(inode), !file->private_data); file->private_data = fp; + if (file->f_mode & FMODE_WRITE) { + ret = gfs2_qa_get(GFS2_I(inode)); + if (ret) + goto fail; + } return 0; + +fail: + kfree(file->private_data); + file->private_data = NULL; + return ret; } /** @@ -690,10 +701,8 @@ static int gfs2_release(struct inode *inode, struct file *file) kfree(file->private_data); file->private_data = NULL; - if (!(file->f_mode & FMODE_WRITE)) - return 0; - - gfs2_rsqa_delete(ip, &inode->i_writecount); + if (file->f_mode & FMODE_WRITE) + gfs2_rsqa_delete(ip, &inode->i_writecount); return 0; } @@ -849,7 +858,7 @@ static ssize_t gfs2_file_write_iter(struct kiocb *iocb, struct iov_iter *from) struct gfs2_inode *ip = GFS2_I(inode); ssize_t ret; - ret = gfs2_qa_alloc(ip); + ret = gfs2_qa_get(ip); if (ret) return ret; @@ -860,7 +869,7 @@ static ssize_t gfs2_file_write_iter(struct kiocb *iocb, struct iov_iter *from) ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh); if (ret) - return ret; + goto out; gfs2_glock_dq_uninit(&gh); } @@ -918,6 +927,8 @@ static ssize_t gfs2_file_write_iter(struct kiocb *iocb, struct iov_iter *from) out_unlock: inode_unlock(inode); +out: + gfs2_qa_put(ip); return ret; } @@ -1149,7 +1160,7 @@ static long gfs2_fallocate(struct file *file, int mode, loff_t offset, loff_t le if (mode & FALLOC_FL_PUNCH_HOLE) { ret = __gfs2_punch_hole(file, offset, len); } else { - ret = gfs2_qa_alloc(ip); + ret = gfs2_qa_get(ip); if (ret) goto out_putw; @@ -1157,6 +1168,7 @@ static long gfs2_fallocate(struct file *file, int mode, loff_t offset, loff_t le if (ret) gfs2_rs_deltree(&ip->i_res); + gfs2_qa_put(ip); } out_putw: @@ -1175,14 +1187,17 @@ static ssize_t gfs2_file_splice_write(struct pipe_inode_info *pipe, { int error; struct gfs2_inode *ip = GFS2_I(out->f_mapping->host); + ssize_t ret; - error = gfs2_qa_alloc(ip); + error = gfs2_qa_get(ip); if (error) return (ssize_t)error; gfs2_size_hint(out, *ppos, len); - return iter_file_splice_write(pipe, out, ppos, len, flags); + ret = iter_file_splice_write(pipe, out, ppos, len, flags); + gfs2_qa_put(ip); + return ret; } #ifdef CONFIG_GFS2_FS_LOCKING_DLM diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h index 04549a8cae7e..84a824293a78 100644 --- a/fs/gfs2/incore.h +++ b/fs/gfs2/incore.h @@ -295,6 +295,7 @@ struct gfs2_qadata { /* quota allocation data */ struct gfs2_quota_data *qa_qd[2 * GFS2_MAXQUOTAS]; struct gfs2_holder qa_qd_ghs[2 * GFS2_MAXQUOTAS]; unsigned int qa_qd_num; + int qa_ref; }; /* Resource group multi-block reservation, in order of appearance: diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 028c272911f6..6c5f527dafc4 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -594,13 +594,13 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, if (!name->len || name->len > GFS2_FNAMESIZE) return -ENAMETOOLONG; - error = gfs2_qa_alloc(dip); + error = gfs2_qa_get(dip); if (error) return error; error = gfs2_rindex_update(sdp); if (error) - return error; + goto fail; error = gfs2_glock_nq_init(dip->i_gl, LM_ST_EXCLUSIVE, 0, ghs); if (error) @@ -647,7 +647,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, goto fail_gunlock; ip = GFS2_I(inode); - error = gfs2_qa_alloc(ip); + error = gfs2_qa_get(ip); if (error) goto fail_free_acls; @@ -782,6 +782,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, clear_bit(GLF_INODE_CREATING, &io_gl->gl_flags); gfs2_glock_put(io_gl); fail_free_inode: + gfs2_qa_put(ip); if (ip->i_gl) { glock_clear_object(ip->i_gl, ip); gfs2_glock_put(ip->i_gl); @@ -804,6 +805,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, if (gfs2_holder_initialized(ghs + 1)) gfs2_glock_dq_uninit(ghs + 1); fail: + gfs2_qa_put(dip); return error; } @@ -905,7 +907,7 @@ static int gfs2_link(struct dentry *old_dentry, struct inode *dir, if (S_ISDIR(inode->i_mode)) return -EPERM; - error = gfs2_qa_alloc(dip); + error = gfs2_qa_get(dip); if (error) return error; @@ -1008,6 +1010,7 @@ static int gfs2_link(struct dentry *old_dentry, struct inode *dir, out_child: gfs2_glock_dq(ghs); out_parent: + gfs2_qa_put(ip); gfs2_holder_uninit(ghs); gfs2_holder_uninit(ghs + 1); return error; @@ -1368,7 +1371,7 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, if (error) return error; - error = gfs2_qa_alloc(ndip); + error = gfs2_qa_get(ndip); if (error) return error; @@ -1568,6 +1571,7 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, if (gfs2_holder_initialized(&r_gh)) gfs2_glock_dq_uninit(&r_gh); out: + gfs2_qa_put(ndip); return error; } @@ -1879,10 +1883,9 @@ static int setattr_chown(struct inode *inode, struct iattr *attr) ouid = nuid = NO_UID_QUOTA_CHANGE; if (!(attr->ia_valid & ATTR_GID) || gid_eq(ogid, ngid)) ogid = ngid = NO_GID_QUOTA_CHANGE; - - error = gfs2_qa_alloc(ip); + error = gfs2_qa_get(ip); if (error) - goto out; + return error; error = gfs2_rindex_update(sdp); if (error) @@ -1920,6 +1923,7 @@ static int setattr_chown(struct inode *inode, struct iattr *attr) out_gunlock_q: gfs2_quota_unlock(ip); out: + gfs2_qa_put(ip); return error; } @@ -1941,21 +1945,21 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr) struct gfs2_holder i_gh; int error; - error = gfs2_qa_alloc(ip); + error = gfs2_qa_get(ip); if (error) return error; error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &i_gh); if (error) - return error; + goto out; error = -EPERM; if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) - goto out; + goto error; error = setattr_prepare(dentry, attr); if (error) - goto out; + goto error; if (attr->ia_valid & ATTR_SIZE) error = gfs2_setattr_size(inode, attr->ia_size); @@ -1967,10 +1971,12 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr) error = posix_acl_chmod(inode, inode->i_mode); } -out: +error: if (!error) mark_inode_dirty(inode); gfs2_glock_dq_uninit(&i_gh); +out: + gfs2_qa_put(ip); return error; } diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c index 6ec7b1dcd81a..c51ed8ddaff0 100644 --- a/fs/gfs2/quota.c +++ b/fs/gfs2/quota.c @@ -525,11 +525,11 @@ static void qdsb_put(struct gfs2_quota_data *qd) } /** - * gfs2_qa_alloc - make sure we have a quota allocations data structure, - * if necessary + * gfs2_qa_get - make sure we have a quota allocations data structure, + * if necessary * @ip: the inode for this reservation */ -int gfs2_qa_alloc(struct gfs2_inode *ip) +int gfs2_qa_get(struct gfs2_inode *ip) { int error = 0; struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode); @@ -540,17 +540,21 @@ int gfs2_qa_alloc(struct gfs2_inode *ip) down_write(&ip->i_rw_mutex); if (ip->i_qadata == NULL) { ip->i_qadata = kmem_cache_zalloc(gfs2_qadata_cachep, GFP_NOFS); - if (!ip->i_qadata) + if (!ip->i_qadata) { error = -ENOMEM; + goto out; + } } + ip->i_qadata->qa_ref++; +out: up_write(&ip->i_rw_mutex); return error; } -void gfs2_qa_delete(struct gfs2_inode *ip, atomic_t *wcount) +void gfs2_qa_put(struct gfs2_inode *ip) { down_write(&ip->i_rw_mutex); - if (ip->i_qadata && ((wcount == NULL) || (atomic_read(wcount) <= 1))) { + if (ip->i_qadata && --ip->i_qadata->qa_ref == 0) { kmem_cache_free(gfs2_qadata_cachep, ip->i_qadata); ip->i_qadata = NULL; } @@ -566,27 +570,27 @@ int gfs2_quota_hold(struct gfs2_inode *ip, kuid_t uid, kgid_t gid) if (sdp->sd_args.ar_quota == GFS2_QUOTA_OFF) return 0; - if (ip->i_qadata == NULL) { - error = gfs2_qa_alloc(ip); - if (error) - return error; - } + error = gfs2_qa_get(ip); + if (error) + return error; qd = ip->i_qadata->qa_qd; if (gfs2_assert_warn(sdp, !ip->i_qadata->qa_qd_num) || - gfs2_assert_warn(sdp, !test_bit(GIF_QD_LOCKED, &ip->i_flags))) - return -EIO; + gfs2_assert_warn(sdp, !test_bit(GIF_QD_LOCKED, &ip->i_flags))) { + error = -EIO; + goto out; + } error = qdsb_get(sdp, make_kqid_uid(ip->i_inode.i_uid), qd); if (error) - goto out; + goto out_unhold; ip->i_qadata->qa_qd_num++; qd++; error = qdsb_get(sdp, make_kqid_gid(ip->i_inode.i_gid), qd); if (error) - goto out; + goto out_unhold; ip->i_qadata->qa_qd_num++; qd++; @@ -594,7 +598,7 @@ int gfs2_quota_hold(struct gfs2_inode *ip, kuid_t uid, kgid_t gid) !uid_eq(uid, ip->i_inode.i_uid)) { error = qdsb_get(sdp, make_kqid_uid(uid), qd); if (error) - goto out; + goto out_unhold; ip->i_qadata->qa_qd_num++; qd++; } @@ -603,14 +607,15 @@ int gfs2_quota_hold(struct gfs2_inode *ip, kuid_t uid, kgid_t gid) !gid_eq(gid, ip->i_inode.i_gid)) { error = qdsb_get(sdp, make_kqid_gid(gid), qd); if (error) - goto out; + goto out_unhold; ip->i_qadata->qa_qd_num++; qd++; } -out: +out_unhold: if (error) gfs2_quota_unhold(ip); +out: return error; } @@ -621,6 +626,7 @@ void gfs2_quota_unhold(struct gfs2_inode *ip) if (ip->i_qadata == NULL) return; + gfs2_assert_warn(sdp, !test_bit(GIF_QD_LOCKED, &ip->i_flags)); for (x = 0; x < ip->i_qadata->qa_qd_num; x++) { @@ -628,6 +634,7 @@ void gfs2_quota_unhold(struct gfs2_inode *ip) ip->i_qadata->qa_qd[x] = NULL; } ip->i_qadata->qa_qd_num = 0; + gfs2_qa_put(ip); } static int sort_qd(const void *a, const void *b) @@ -876,7 +883,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda) unsigned int nalloc = 0, blocks; int error; - error = gfs2_qa_alloc(ip); + error = gfs2_qa_get(ip); if (error) return error; @@ -884,8 +891,10 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda) &data_blocks, &ind_blocks); ghs = kmalloc_array(num_qd, sizeof(struct gfs2_holder), GFP_NOFS); - if (!ghs) - return -ENOMEM; + if (!ghs) { + error = -ENOMEM; + goto out; + } sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL); inode_lock(&ip->i_inode); @@ -893,12 +902,12 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda) error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE, GL_NOCACHE, &ghs[qx]); if (error) - goto out; + goto out_dq; } error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &i_gh); if (error) - goto out; + goto out_dq; for (x = 0; x < num_qd; x++) { offset = qd2offset(qda[x]); @@ -950,13 +959,15 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda) gfs2_inplace_release(ip); out_alloc: gfs2_glock_dq_uninit(&i_gh); -out: +out_dq: while (qx--) gfs2_glock_dq_uninit(&ghs[qx]); inode_unlock(&ip->i_inode); kfree(ghs); gfs2_log_flush(ip->i_gl->gl_name.ln_sbd, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL | GFS2_LFC_DO_SYNC); +out: + gfs2_qa_put(ip); return error; } @@ -1259,6 +1270,7 @@ void gfs2_quota_change(struct gfs2_inode *ip, s64 change, if (ip->i_diskflags & GFS2_DIF_SYSTEM) return; + BUG_ON(ip->i_qadata->qa_ref <= 0); for (x = 0; x < ip->i_qadata->qa_qd_num; x++) { qd = ip->i_qadata->qa_qd[x]; @@ -1677,7 +1689,7 @@ static int gfs2_set_dqblk(struct super_block *sb, struct kqid qid, if (error) return error; - error = gfs2_qa_alloc(ip); + error = gfs2_qa_get(ip); if (error) goto out_put; @@ -1746,6 +1758,7 @@ static int gfs2_set_dqblk(struct super_block *sb, struct kqid qid, out_q: gfs2_glock_dq_uninit(&q_gh); out_unlockput: + gfs2_qa_put(ip); inode_unlock(&ip->i_inode); out_put: qd_put(qd); diff --git a/fs/gfs2/quota.h b/fs/gfs2/quota.h index 765627d9a91e..7f9ca8ef40fc 100644 --- a/fs/gfs2/quota.h +++ b/fs/gfs2/quota.h @@ -15,8 +15,8 @@ struct gfs2_sbd; #define NO_UID_QUOTA_CHANGE INVALID_UID #define NO_GID_QUOTA_CHANGE INVALID_GID -extern int gfs2_qa_alloc(struct gfs2_inode *ip); -extern void gfs2_qa_delete(struct gfs2_inode *ip, atomic_t *wcount); +extern int gfs2_qa_get(struct gfs2_inode *ip); +extern void gfs2_qa_put(struct gfs2_inode *ip); extern int gfs2_quota_hold(struct gfs2_inode *ip, kuid_t uid, kgid_t gid); extern void gfs2_quota_unhold(struct gfs2_inode *ip); diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c index 3e3696da5bcb..04e3e13a230c 100644 --- a/fs/gfs2/rgrp.c +++ b/fs/gfs2/rgrp.c @@ -673,7 +673,7 @@ void gfs2_rsqa_delete(struct gfs2_inode *ip, atomic_t *wcount) if ((wcount == NULL) || (atomic_read(wcount) <= 1)) gfs2_rs_deltree(&ip->i_res); up_write(&ip->i_rw_mutex); - gfs2_qa_delete(ip, wcount); + gfs2_qa_put(ip); } /** diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c index 693c6d13473c..709dd80d4ebe 100644 --- a/fs/gfs2/super.c +++ b/fs/gfs2/super.c @@ -1409,6 +1409,8 @@ static void gfs2_evict_inode(struct inode *inode) fs_warn(sdp, "gfs2_evict_inode: %d\n", error); out: truncate_inode_pages_final(&inode->i_data); + if (ip->i_qadata) + gfs2_assert_warn(sdp, ip->i_qadata->qa_ref == 0); gfs2_rsqa_delete(ip, NULL); gfs2_ordered_del_inode(ip); clear_inode(inode); diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c index c4fbb96e001f..9d7667bc4292 100644 --- a/fs/gfs2/xattr.c +++ b/fs/gfs2/xattr.c @@ -1222,7 +1222,7 @@ static int gfs2_xattr_set(const struct xattr_handler *handler, struct gfs2_holder gh; int ret; - ret = gfs2_qa_alloc(ip); + ret = gfs2_qa_get(ip); if (ret) return ret; @@ -1231,15 +1231,19 @@ static int gfs2_xattr_set(const struct xattr_handler *handler, if (!gfs2_glock_is_locked_by_me(ip->i_gl)) { ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh); if (ret) - return ret; + goto out; } else { - if (WARN_ON_ONCE(ip->i_gl->gl_state != LM_ST_EXCLUSIVE)) - return -EIO; + if (WARN_ON_ONCE(ip->i_gl->gl_state != LM_ST_EXCLUSIVE)) { + ret = -EIO; + goto out; + } gfs2_holder_mark_uninitialized(&gh); } ret = __gfs2_xattr_set(inode, name, value, size, flags, handler->flags); if (gfs2_holder_initialized(&gh)) gfs2_glock_dq_uninit(&gh); +out: + gfs2_qa_put(ip); return ret; } -- 2.25.1