[RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Luis Henriques]

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 result in an immediate
notification of the MDSs with the new file size, and thus the quota realms
stats won't be updated.

This patch forces a sync with the MDS every time there's an ATTR_SIZE that
sets a new i_size, even if we have Fx caps.

Cc: stable@vger.kernel.org
Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
URL: https://tracker.ceph.com/issues/36593
Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 fs/ceph/inode.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 526faf4778ce..30e3f240ac96 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	if (ia_valid & ATTR_SIZE) {
 		dout("setattr %p size %lld -> %lld\n", inode,
 		     inode->i_size, attr->ia_size);
-		if ((issued & CEPH_CAP_FILE_EXCL) &&
-		    attr->ia_size > inode->i_size) {
-			i_size_write(inode, attr->ia_size);
-			inode->i_blocks = calc_inode_blocks(attr->ia_size);
-			ci->i_reported_size = attr->ia_size;
-			dirtied |= CEPH_CAP_FILE_EXCL;
-			ia_valid |= ATTR_MTIME;
-		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
-			   attr->ia_size != inode->i_size) {
+		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
+		    (attr->ia_size != inode->i_size)) {
 			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
 			req->r_args.setattr.old_size =
 				cpu_to_le64(inode->i_size);

Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Jeff Layton]

On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
> 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 result in an immediate
> notification of the MDSs with the new file size, and thus the quota realms
> stats won't be updated.
> 
> This patch forces a sync with the MDS every time there's an ATTR_SIZE that
> sets a new i_size, even if we have Fx caps.
> 
> Cc: stable@vger.kernel.org
> Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
> URL: https://tracker.ceph.com/issues/36593
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  fs/ceph/inode.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 526faf4778ce..30e3f240ac96 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>  	if (ia_valid & ATTR_SIZE) {
>  		dout("setattr %p size %lld -> %lld\n", inode,
>  		     inode->i_size, attr->ia_size);
> -		if ((issued & CEPH_CAP_FILE_EXCL) &&
> -		    attr->ia_size > inode->i_size) {
> -			i_size_write(inode, attr->ia_size);
> -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
> -			ci->i_reported_size = attr->ia_size;
> -			dirtied |= CEPH_CAP_FILE_EXCL;
> -			ia_valid |= ATTR_MTIME;
> -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
> -			   attr->ia_size != inode->i_size) {
> +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
> +		    (attr->ia_size != inode->i_size)) {
>  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
>  			req->r_args.setattr.old_size =
>  				cpu_to_le64(inode->i_size);

Hmm...this makes truncates more expensive when we have caps. I'd rather
not do that if we can help it.

What about instead having the client mimic a fsync when there is a
rename across quota realms? If we can't tell that reliably then we could
also just do an effective fsync ahead of any cross-directory rename?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Luis Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
>> 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 result in an immediate
>> notification of the MDSs with the new file size, and thus the quota realms
>> stats won't be updated.
>> 
>> This patch forces a sync with the MDS every time there's an ATTR_SIZE that
>> sets a new i_size, even if we have Fx caps.
>> 
>> Cc: stable@vger.kernel.org
>> Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
>> URL: https://tracker.ceph.com/issues/36593
>> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> ---
>>  fs/ceph/inode.c | 11 ++---------
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>> 
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 526faf4778ce..30e3f240ac96 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>>  	if (ia_valid & ATTR_SIZE) {
>>  		dout("setattr %p size %lld -> %lld\n", inode,
>>  		     inode->i_size, attr->ia_size);
>> -		if ((issued & CEPH_CAP_FILE_EXCL) &&
>> -		    attr->ia_size > inode->i_size) {
>> -			i_size_write(inode, attr->ia_size);
>> -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
>> -			ci->i_reported_size = attr->ia_size;
>> -			dirtied |= CEPH_CAP_FILE_EXCL;
>> -			ia_valid |= ATTR_MTIME;
>> -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
>> -			   attr->ia_size != inode->i_size) {
>> +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
>> +		    (attr->ia_size != inode->i_size)) {
>>  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
>>  			req->r_args.setattr.old_size =
>>  				cpu_to_le64(inode->i_size);
>
> Hmm...this makes truncates more expensive when we have caps. I'd rather
> not do that if we can help it.

Yeah, as I mentioned in the tracker, there's indeed a performance impact
with this fix.  That's what made me add the RFC in the subject ;-)

> What about instead having the client mimic a fsync when there is a
> rename across quota realms? If we can't tell that reliably then we could
> also just do an effective fsync ahead of any cross-directory rename?

Ok, thanks for the suggestion.  That may actually work, although it will
make the rename more expensive of course.  I'll test that tomorrow and
eventually follow-up with a patch.

Cheers,
-- 
Luis

Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Jeff Layton]

On Wed, 2020-11-11 at 18:28 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
> > > 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 result in an immediate
> > > notification of the MDSs with the new file size, and thus the quota realms
> > > stats won't be updated.
> > > 
> > > This patch forces a sync with the MDS every time there's an ATTR_SIZE that
> > > sets a new i_size, even if we have Fx caps.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
> > > URL: https://tracker.ceph.com/issues/36593
> > > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > > ---
> > >  fs/ceph/inode.c | 11 ++---------
> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index 526faf4778ce..30e3f240ac96 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > >  	if (ia_valid & ATTR_SIZE) {
> > >  		dout("setattr %p size %lld -> %lld\n", inode,
> > >  		     inode->i_size, attr->ia_size);
> > > -		if ((issued & CEPH_CAP_FILE_EXCL) &&
> > > -		    attr->ia_size > inode->i_size) {
> > > -			i_size_write(inode, attr->ia_size);
> > > -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
> > > -			ci->i_reported_size = attr->ia_size;
> > > -			dirtied |= CEPH_CAP_FILE_EXCL;
> > > -			ia_valid |= ATTR_MTIME;
> > > -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
> > > -			   attr->ia_size != inode->i_size) {
> > > +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
> > > +		    (attr->ia_size != inode->i_size)) {
> > >  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
> > >  			req->r_args.setattr.old_size =
> > >  				cpu_to_le64(inode->i_size);
> > 
> > Hmm...this makes truncates more expensive when we have caps. I'd rather
> > not do that if we can help it.
> 
> Yeah, as I mentioned in the tracker, there's indeed a performance impact
> with this fix.  That's what made me add the RFC in the subject ;-)
> 
> > What about instead having the client mimic a fsync when there is a
> > rename across quota realms? If we can't tell that reliably then we could
> > also just do an effective fsync ahead of any cross-directory rename?
> 
> Ok, thanks for the suggestion.  That may actually work, although it will
> make the rename more expensive of course.  I'll test that tomorrow and
> eventually follow-up with a patch.

In principle, there should only be an impact when the file being renamed
has dirty data and is crossing quota realms. I'd much rather slow down
the rename than truncate in this case. open(..., O_TRUNC) is _very_
common.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Jeff Layton]

On Wed, 2020-11-11 at 18:28 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
> > > 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 result in an immediate
> > > notification of the MDSs with the new file size, and thus the quota realms
> > > stats won't be updated.
> > > 
> > > This patch forces a sync with the MDS every time there's an ATTR_SIZE that
> > > sets a new i_size, even if we have Fx caps.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
> > > URL: https://tracker.ceph.com/issues/36593
> > > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > > ---
> > >  fs/ceph/inode.c | 11 ++---------
> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index 526faf4778ce..30e3f240ac96 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > >  	if (ia_valid & ATTR_SIZE) {
> > >  		dout("setattr %p size %lld -> %lld\n", inode,
> > >  		     inode->i_size, attr->ia_size);
> > > -		if ((issued & CEPH_CAP_FILE_EXCL) &&
> > > -		    attr->ia_size > inode->i_size) {
> > > -			i_size_write(inode, attr->ia_size);
> > > -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
> > > -			ci->i_reported_size = attr->ia_size;
> > > -			dirtied |= CEPH_CAP_FILE_EXCL;
> > > -			ia_valid |= ATTR_MTIME;
> > > -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
> > > -			   attr->ia_size != inode->i_size) {
> > > +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
> > > +		    (attr->ia_size != inode->i_size)) {
> > >  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
> > >  			req->r_args.setattr.old_size =
> > >  				cpu_to_le64(inode->i_size);
> > 
> > Hmm...this makes truncates more expensive when we have caps. I'd rather
> > not do that if we can help it.
> 
> Yeah, as I mentioned in the tracker, there's indeed a performance impact
> with this fix.  That's what made me add the RFC in the subject ;-)
> 
> > What about instead having the client mimic a fsync when there is a
> > rename across quota realms? If we can't tell that reliably then we could
> > also just do an effective fsync ahead of any cross-directory rename?
> 
> Ok, thanks for the suggestion.  That may actually work, although it will
> make the rename more expensive of course.  I'll test that tomorrow and
> eventually follow-up with a patch.
> 

Patrick pointed out to me on IRC that since you're moving the parent
directory of the truncated file, flushing the caps on the directory
won't really help. You'd need to walk the entire subtree and try to
flush every dirty inode, or basically do a syncfs() prior to renaming
the directory across quotarealms.

I think we probably will need to revert the change to allow cross-
quotarealm renames of directories and make those return EXDEV again.
Anything else sounds like it's probably going to be too expensive.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Luis Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> On Wed, 2020-11-11 at 18:28 +0000, Luis Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> writes:
>> 
>> > On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
>> > > 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 result in an immediate
>> > > notification of the MDSs with the new file size, and thus the quota realms
>> > > stats won't be updated.
>> > > 
>> > > This patch forces a sync with the MDS every time there's an ATTR_SIZE that
>> > > sets a new i_size, even if we have Fx caps.
>> > > 
>> > > Cc: stable@vger.kernel.org
>> > > Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
>> > > URL: https://tracker.ceph.com/issues/36593
>> > > Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> > > ---
>> > >  fs/ceph/inode.c | 11 ++---------
>> > >  1 file changed, 2 insertions(+), 9 deletions(-)
>> > > 
>> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> > > index 526faf4778ce..30e3f240ac96 100644
>> > > --- a/fs/ceph/inode.c
>> > > +++ b/fs/ceph/inode.c
>> > > @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>> > >  	if (ia_valid & ATTR_SIZE) {
>> > >  		dout("setattr %p size %lld -> %lld\n", inode,
>> > >  		     inode->i_size, attr->ia_size);
>> > > -		if ((issued & CEPH_CAP_FILE_EXCL) &&
>> > > -		    attr->ia_size > inode->i_size) {
>> > > -			i_size_write(inode, attr->ia_size);
>> > > -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
>> > > -			ci->i_reported_size = attr->ia_size;
>> > > -			dirtied |= CEPH_CAP_FILE_EXCL;
>> > > -			ia_valid |= ATTR_MTIME;
>> > > -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
>> > > -			   attr->ia_size != inode->i_size) {
>> > > +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
>> > > +		    (attr->ia_size != inode->i_size)) {
>> > >  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
>> > >  			req->r_args.setattr.old_size =
>> > >  				cpu_to_le64(inode->i_size);
>> > 
>> > Hmm...this makes truncates more expensive when we have caps. I'd rather
>> > not do that if we can help it.
>> 
>> Yeah, as I mentioned in the tracker, there's indeed a performance impact
>> with this fix.  That's what made me add the RFC in the subject ;-)
>> 
>> > What about instead having the client mimic a fsync when there is a
>> > rename across quota realms? If we can't tell that reliably then we could
>> > also just do an effective fsync ahead of any cross-directory rename?
>> 
>> Ok, thanks for the suggestion.  That may actually work, although it will
>> make the rename more expensive of course.  I'll test that tomorrow and
>> eventually follow-up with a patch.
>> 
>
> Patrick pointed out to me on IRC that since you're moving the parent
> directory of the truncated file, flushing the caps on the directory
> won't really help. You'd need to walk the entire subtree and try to
> flush every dirty inode, or basically do a syncfs() prior to renaming
> the directory across quotarealms.
>
> I think we probably will need to revert the change to allow cross-
> quotarealm renames of directories and make those return EXDEV again.
> Anything else sounds like it's probably going to be too expensive.

Hmm... that sounds a bit drastic and it would make the kernel client
behave differently from the fuse client -- from what I could understand
the fuse client does the sync ATTR_SIZE and thus doesn't have this issue.

Obviously, I agree with you that the performance penalty is too high for
such a common operation.  But maybe renames across quotarealms aren't that
common and paying the penalty of doing a full ceph_flush_dirty_caps() is
acceptable for such cases?

Cheers,
-- 
Luis

Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Jeff Layton]

On Thu, 2020-11-12 at 10:40 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > On Wed, 2020-11-11 at 18:28 +0000, Luis Henriques wrote:
> > > Jeff Layton <jlayton@kernel.org> writes:
> > > 
> > > > On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
> > > > > 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 result in an immediate
> > > > > notification of the MDSs with the new file size, and thus the quota realms
> > > > > stats won't be updated.
> > > > > 
> > > > > This patch forces a sync with the MDS every time there's an ATTR_SIZE that
> > > > > sets a new i_size, even if we have Fx caps.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
> > > > > URL: https://tracker.ceph.com/issues/36593
> > > > > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > > > > ---
> > > > >  fs/ceph/inode.c | 11 ++---------
> > > > >  1 file changed, 2 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > index 526faf4778ce..30e3f240ac96 100644
> > > > > --- a/fs/ceph/inode.c
> > > > > +++ b/fs/ceph/inode.c
> > > > > @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > > > >  	if (ia_valid & ATTR_SIZE) {
> > > > >  		dout("setattr %p size %lld -> %lld\n", inode,
> > > > >  		     inode->i_size, attr->ia_size);
> > > > > -		if ((issued & CEPH_CAP_FILE_EXCL) &&
> > > > > -		    attr->ia_size > inode->i_size) {
> > > > > -			i_size_write(inode, attr->ia_size);
> > > > > -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
> > > > > -			ci->i_reported_size = attr->ia_size;
> > > > > -			dirtied |= CEPH_CAP_FILE_EXCL;
> > > > > -			ia_valid |= ATTR_MTIME;
> > > > > -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
> > > > > -			   attr->ia_size != inode->i_size) {
> > > > > +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
> > > > > +		    (attr->ia_size != inode->i_size)) {
> > > > >  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
> > > > >  			req->r_args.setattr.old_size =
> > > > >  				cpu_to_le64(inode->i_size);
> > > > 
> > > > Hmm...this makes truncates more expensive when we have caps. I'd rather
> > > > not do that if we can help it.
> > > 
> > > Yeah, as I mentioned in the tracker, there's indeed a performance impact
> > > with this fix.  That's what made me add the RFC in the subject ;-)
> > > 
> > > > What about instead having the client mimic a fsync when there is a
> > > > rename across quota realms? If we can't tell that reliably then we could
> > > > also just do an effective fsync ahead of any cross-directory rename?
> > > 
> > > Ok, thanks for the suggestion.  That may actually work, although it will
> > > make the rename more expensive of course.  I'll test that tomorrow and
> > > eventually follow-up with a patch.
> > > 
> > 
> > Patrick pointed out to me on IRC that since you're moving the parent
> > directory of the truncated file, flushing the caps on the directory
> > won't really help. You'd need to walk the entire subtree and try to
> > flush every dirty inode, or basically do a syncfs() prior to renaming
> > the directory across quotarealms.
> > 
> > I think we probably will need to revert the change to allow cross-
> > quotarealm renames of directories and make those return EXDEV again.
> > Anything else sounds like it's probably going to be too expensive.
> 
> Hmm... that sounds a bit drastic and it would make the kernel client
> behave differently from the fuse client -- from what I could understand
> the fuse client does the sync ATTR_SIZE and thus doesn't have this issue.
> 

True. I'll note that the fuse client is not exactly built for speed,
however.

> Obviously, I agree with you that the performance penalty is too high for
> such a common operation.  But maybe renames across quotarealms aren't that
> common and paying the penalty of doing a full ceph_flush_dirty_caps() is
> acceptable for such cases?
> 

I wouldn't even do that. If someone is renaming a directory across
quotarealms, just return EXDEV. Saying "sorry, you have to copy/unlink"
in this situation seems like it should be acceptable. Are you aware of
any specific use-cases where people are renaming large directories
across quotarealms?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

[Luis Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> On Thu, 2020-11-12 at 10:40 +0000, Luis Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> writes:
>> 
>> > On Wed, 2020-11-11 at 18:28 +0000, Luis Henriques wrote:
>> > > Jeff Layton <jlayton@kernel.org> writes:
>> > > 
>> > > > On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
>> > > > > 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 result in an immediate
>> > > > > notification of the MDSs with the new file size, and thus the quota realms
>> > > > > stats won't be updated.
>> > > > > 
>> > > > > This patch forces a sync with the MDS every time there's an ATTR_SIZE that
>> > > > > sets a new i_size, even if we have Fx caps.
>> > > > > 
>> > > > > Cc: stable@vger.kernel.org
>> > > > > Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
>> > > > > URL: https://tracker.ceph.com/issues/36593
>> > > > > Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> > > > > ---
>> > > > >  fs/ceph/inode.c | 11 ++---------
>> > > > >  1 file changed, 2 insertions(+), 9 deletions(-)
>> > > > > 
>> > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> > > > > index 526faf4778ce..30e3f240ac96 100644
>> > > > > --- a/fs/ceph/inode.c
>> > > > > +++ b/fs/ceph/inode.c
>> > > > > @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>> > > > >  	if (ia_valid & ATTR_SIZE) {
>> > > > >  		dout("setattr %p size %lld -> %lld\n", inode,
>> > > > >  		     inode->i_size, attr->ia_size);
>> > > > > -		if ((issued & CEPH_CAP_FILE_EXCL) &&
>> > > > > -		    attr->ia_size > inode->i_size) {
>> > > > > -			i_size_write(inode, attr->ia_size);
>> > > > > -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
>> > > > > -			ci->i_reported_size = attr->ia_size;
>> > > > > -			dirtied |= CEPH_CAP_FILE_EXCL;
>> > > > > -			ia_valid |= ATTR_MTIME;
>> > > > > -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
>> > > > > -			   attr->ia_size != inode->i_size) {
>> > > > > +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
>> > > > > +		    (attr->ia_size != inode->i_size)) {
>> > > > >  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
>> > > > >  			req->r_args.setattr.old_size =
>> > > > >  				cpu_to_le64(inode->i_size);
>> > > > 
>> > > > Hmm...this makes truncates more expensive when we have caps. I'd rather
>> > > > not do that if we can help it.
>> > > 
>> > > Yeah, as I mentioned in the tracker, there's indeed a performance impact
>> > > with this fix.  That's what made me add the RFC in the subject ;-)
>> > > 
>> > > > What about instead having the client mimic a fsync when there is a
>> > > > rename across quota realms? If we can't tell that reliably then we could
>> > > > also just do an effective fsync ahead of any cross-directory rename?
>> > > 
>> > > Ok, thanks for the suggestion.  That may actually work, although it will
>> > > make the rename more expensive of course.  I'll test that tomorrow and
>> > > eventually follow-up with a patch.
>> > > 
>> > 
>> > Patrick pointed out to me on IRC that since you're moving the parent
>> > directory of the truncated file, flushing the caps on the directory
>> > won't really help. You'd need to walk the entire subtree and try to
>> > flush every dirty inode, or basically do a syncfs() prior to renaming
>> > the directory across quotarealms.
>> > 
>> > I think we probably will need to revert the change to allow cross-
>> > quotarealm renames of directories and make those return EXDEV again.
>> > Anything else sounds like it's probably going to be too expensive.
>> 
>> Hmm... that sounds a bit drastic and it would make the kernel client
>> behave differently from the fuse client -- from what I could understand
>> the fuse client does the sync ATTR_SIZE and thus doesn't have this issue.
>> 
>
> True. I'll note that the fuse client is not exactly built for speed,
> however.
>
>> Obviously, I agree with you that the performance penalty is too high for
>> such a common operation.  But maybe renames across quotarealms aren't that
>> common and paying the penalty of doing a full ceph_flush_dirty_caps() is
>> acceptable for such cases?
>> 
>
> I wouldn't even do that. If someone is renaming a directory across
> quotarealms, just return EXDEV. Saying "sorry, you have to copy/unlink"
> in this situation seems like it should be acceptable. Are you aware of
> any specific use-cases where people are renaming large directories
> across quotarealms?

No, no specific user-cases I'm aware of.  The reasoning was simply an
issue[1] in the tracker created by Greg.  Basically fuse client commit
b8954e5734b3 ("client: optimize rename operation under different quota
root"), which has it's own issues[2][3] associated, triggered the
implementation of the same behaviour on the kernel client.

Anyway, I'm send the revert of dffdcd71458e ("ceph: allow rename operation
under different quota realms") in a second.

[1] https://tracker.ceph.com/issues/44791
[2] https://tracker.ceph.com/issues/39715
[3] https://tracker.ceph.com/issues/16884

Cheers,
-- 
Luis

[PATCH] Revert "ceph: allow rename operation under different quota realms"

[Luis Henriques]

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 */

Re: [PATCH] Revert "ceph: allow rename operation under different quota realms"

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>