| Message ID | 20201112152321.32491-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "ceph: allow rename operation under different quota realms" | expand |
On Thu, 2020-11-12 at 15:23 +0000, Luis Henriques wrote: > This reverts commit dffdcd71458e699e839f0bf47c3d42d64210b939. > > When doing a rename across quota realms, there's a corner case that isn't > handled correctly. Here's a testcase: > > mkdir files limit > truncate files/file -s 10G > setfattr limit -n ceph.quota.max_bytes -v 1000000 > mv files limit/ > > The above will succeed because ftruncate(2) won't immediately notify the > MDSs with the new file size, and thus the quota realms stats won't be > updated. > > Since the possible fixes for this issue would have a huge performance impact, > the solution for now is to simply revert to returning -EXDEV when doing a cross > quota realms rename. > > URL: https://tracker.ceph.com/issues/48203 > Signed-off-by: Luis Henriques <lhenriques@suse.de> > --- > fs/ceph/dir.c | 9 ++++---- > fs/ceph/quota.c | 58 +------------------------------------------------ > fs/ceph/super.h | 3 +-- > 3 files changed, 6 insertions(+), 64 deletions(-) > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index a4d48370b2b3..858ee7362ff5 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1202,12 +1202,11 @@ static int ceph_rename(struct inode *old_dir, struct dentry *old_dentry, > op = CEPH_MDS_OP_RENAMESNAP; > else > return -EROFS; > - } else if (old_dir != new_dir) { > - err = ceph_quota_check_rename(mdsc, d_inode(old_dentry), > - new_dir); > - if (err) > - return err; > } > + /* don't allow cross-quota renames */ > + if ((old_dir != new_dir) && > + (!ceph_quota_is_same_realm(old_dir, new_dir))) > + return -EXDEV; > > > dout("rename dir %p dentry %p to dir %p dentry %p\n", > old_dir, old_dentry, new_dir, new_dentry); > diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c > index 9b785f11e95a..4e32c9600ecc 100644 > --- a/fs/ceph/quota.c > +++ b/fs/ceph/quota.c > @@ -264,7 +264,7 @@ static struct ceph_snap_realm *get_quota_realm(struct ceph_mds_client *mdsc, > return NULL; > } > > > -static bool ceph_quota_is_same_realm(struct inode *old, struct inode *new) > +bool ceph_quota_is_same_realm(struct inode *old, struct inode *new) > { > struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(old->i_sb); > struct ceph_snap_realm *old_realm, *new_realm; > @@ -516,59 +516,3 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf) > return is_updated; > } > > > -/* > - * ceph_quota_check_rename - check if a rename can be executed > - * @mdsc: MDS client instance > - * @old: inode to be copied > - * @new: destination inode (directory) > - * > - * This function verifies if a rename (e.g. moving a file or directory) can be > - * executed. It forces an rstat update in the @new target directory (and in the > - * source @old as well, if it's a directory). The actual check is done both for > - * max_files and max_bytes. > - * > - * This function returns 0 if it's OK to do the rename, or, if quotas are > - * exceeded, -EXDEV (if @old is a directory) or -EDQUOT. > - */ > -int ceph_quota_check_rename(struct ceph_mds_client *mdsc, > - struct inode *old, struct inode *new) > -{ > - struct ceph_inode_info *ci_old = ceph_inode(old); > - int ret = 0; > - > - if (ceph_quota_is_same_realm(old, new)) > - return 0; > - > - /* > - * Get the latest rstat for target directory (and for source, if a > - * directory) > - */ > - ret = ceph_do_getattr(new, CEPH_STAT_RSTAT, false); > - if (ret) > - return ret; > - > - if (S_ISDIR(old->i_mode)) { > - ret = ceph_do_getattr(old, CEPH_STAT_RSTAT, false); > - if (ret) > - return ret; > - ret = check_quota_exceeded(new, QUOTA_CHECK_MAX_BYTES_OP, > - ci_old->i_rbytes); > - if (!ret) > - ret = check_quota_exceeded(new, > - QUOTA_CHECK_MAX_FILES_OP, > - ci_old->i_rfiles + > - ci_old->i_rsubdirs); > - if (ret) > - ret = -EXDEV; > - } else { > - ret = check_quota_exceeded(new, QUOTA_CHECK_MAX_BYTES_OP, > - i_size_read(old)); > - if (!ret) > - ret = check_quota_exceeded(new, > - QUOTA_CHECK_MAX_FILES_OP, 1); > - if (ret) > - ret = -EDQUOT; > - } > - > - return ret; > -} > diff --git a/fs/ceph/super.h b/fs/ceph/super.h > index 482473e4cce1..8dbb0babddea 100644 > --- a/fs/ceph/super.h > +++ b/fs/ceph/super.h > @@ -1222,14 +1222,13 @@ extern void ceph_handle_quota(struct ceph_mds_client *mdsc, > struct ceph_mds_session *session, > struct ceph_msg *msg); > extern bool ceph_quota_is_max_files_exceeded(struct inode *inode); > +extern bool ceph_quota_is_same_realm(struct inode *old, struct inode *new); > extern bool ceph_quota_is_max_bytes_exceeded(struct inode *inode, > loff_t newlen); > extern bool ceph_quota_is_max_bytes_approaching(struct inode *inode, > loff_t newlen); > extern bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, > struct kstatfs *buf); > -extern int ceph_quota_check_rename(struct ceph_mds_client *mdsc, > - struct inode *old, struct inode *new); > extern void ceph_cleanup_quotarealms_inodes(struct ceph_mds_client *mdsc); > > > #endif /* _FS_CEPH_SUPER_H */ Ok, looks reasonable for now. I'll note that we probably _could_ allow for cross-quota renames of regular files since we know that we're only dealing with a single inode. That said, it might be weird to see EXDEV on a directory but not a regular file in the same parent dir.
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index a4d48370b2b3..858ee7362ff5 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1202,12 +1202,11 @@ static int ceph_rename(struct inode *old_dir, struct dentry *old_dentry, op = CEPH_MDS_OP_RENAMESNAP; else return -EROFS; - } else if (old_dir != new_dir) { - err = ceph_quota_check_rename(mdsc, d_inode(old_dentry), - new_dir); - if (err) - return err; } + /* don't allow cross-quota renames */ + if ((old_dir != new_dir) && + (!ceph_quota_is_same_realm(old_dir, new_dir))) + return -EXDEV; dout("rename dir %p dentry %p to dir %p dentry %p\n", old_dir, old_dentry, new_dir, new_dentry); diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c index 9b785f11e95a..4e32c9600ecc 100644 --- a/fs/ceph/quota.c +++ b/fs/ceph/quota.c @@ -264,7 +264,7 @@ static struct ceph_snap_realm *get_quota_realm(struct ceph_mds_client *mdsc, return NULL; } -static bool ceph_quota_is_same_realm(struct inode *old, struct inode *new) +bool ceph_quota_is_same_realm(struct inode *old, struct inode *new) { struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(old->i_sb); struct ceph_snap_realm *old_realm, *new_realm; @@ -516,59 +516,3 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf) return is_updated; } -/* - * ceph_quota_check_rename - check if a rename can be executed - * @mdsc: MDS client instance - * @old: inode to be copied - * @new: destination inode (directory) - * - * This function verifies if a rename (e.g. moving a file or directory) can be - * executed. It forces an rstat update in the @new target directory (and in the - * source @old as well, if it's a directory). The actual check is done both for - * max_files and max_bytes. - * - * This function returns 0 if it's OK to do the rename, or, if quotas are - * exceeded, -EXDEV (if @old is a directory) or -EDQUOT. - */ -int ceph_quota_check_rename(struct ceph_mds_client *mdsc, - struct inode *old, struct inode *new) -{ - struct ceph_inode_info *ci_old = ceph_inode(old); - int ret = 0; - - if (ceph_quota_is_same_realm(old, new)) - return 0; - - /* - * Get the latest rstat for target directory (and for source, if a - * directory) - */ - ret = ceph_do_getattr(new, CEPH_STAT_RSTAT, false); - if (ret) - return ret; - - if (S_ISDIR(old->i_mode)) { - ret = ceph_do_getattr(old, CEPH_STAT_RSTAT, false); - if (ret) - return ret; - ret = check_quota_exceeded(new, QUOTA_CHECK_MAX_BYTES_OP, - ci_old->i_rbytes); - if (!ret) - ret = check_quota_exceeded(new, - QUOTA_CHECK_MAX_FILES_OP, - ci_old->i_rfiles + - ci_old->i_rsubdirs); - if (ret) - ret = -EXDEV; - } else { - ret = check_quota_exceeded(new, QUOTA_CHECK_MAX_BYTES_OP, - i_size_read(old)); - if (!ret) - ret = check_quota_exceeded(new, - QUOTA_CHECK_MAX_FILES_OP, 1); - if (ret) - ret = -EDQUOT; - } - - return ret; -} diff --git a/fs/ceph/super.h b/fs/ceph/super.h index 482473e4cce1..8dbb0babddea 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -1222,14 +1222,13 @@ extern void ceph_handle_quota(struct ceph_mds_client *mdsc, struct ceph_mds_session *session, struct ceph_msg *msg); extern bool ceph_quota_is_max_files_exceeded(struct inode *inode); +extern bool ceph_quota_is_same_realm(struct inode *old, struct inode *new); extern bool ceph_quota_is_max_bytes_exceeded(struct inode *inode, loff_t newlen); extern bool ceph_quota_is_max_bytes_approaching(struct inode *inode, loff_t newlen); extern bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf); -extern int ceph_quota_check_rename(struct ceph_mds_client *mdsc, - struct inode *old, struct inode *new); extern void ceph_cleanup_quotarealms_inodes(struct ceph_mds_client *mdsc); #endif /* _FS_CEPH_SUPER_H */
This reverts commit dffdcd71458e699e839f0bf47c3d42d64210b939. When doing a rename across quota realms, there's a corner case that isn't handled correctly. Here's a testcase: mkdir files limit truncate files/file -s 10G setfattr limit -n ceph.quota.max_bytes -v 1000000 mv files limit/ The above will succeed because ftruncate(2) won't immediately notify the MDSs with the new file size, and thus the quota realms stats won't be updated. Since the possible fixes for this issue would have a huge performance impact, the solution for now is to simply revert to returning -EXDEV when doing a cross quota realms rename. URL: https://tracker.ceph.com/issues/48203 Signed-off-by: Luis Henriques <lhenriques@suse.de> --- fs/ceph/dir.c | 9 ++++---- fs/ceph/quota.c | 58 +------------------------------------------------ fs/ceph/super.h | 3 +-- 3 files changed, 6 insertions(+), 64 deletions(-)