* Re: [RFC PATCH] ceph: disable updating the atime since cephfs won't maintain it
2022-04-20 13:56 ` Jeff Layton
@ 2022-04-20 14:08 ` Gregory Farnum
2022-04-21 1:12 ` Xiubo Li
2022-04-21 1:08 ` Xiubo Li
2022-04-21 7:18 ` Xiubo Li
2 siblings, 1 reply; 7+ messages in thread
From: Gregory Farnum @ 2022-04-20 14:08 UTC (permalink / raw)
To: Jeff Layton; +Cc: Xiubo Li, Ilya Dryomov, Venky Shankar, ceph-devel
On Wed, Apr 20, 2022 at 6:57 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2022-04-20 at 13:24 +0800, Xiubo Li wrote:
> > Since the cephFS makes no attempt to maintain atime, we shouldn't
> > try to update it in mmap and generic read cases and ignore updating
> > it in direct and sync read cases.
> >
> > And even we update it in mmap and generic read cases we will drop
> > it and won't sync it to MDS. And we are seeing the atime will be
> > updated and then dropped to the floor again and again.
> >
> > URL: https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/VSJM7T4CS5TDRFF6XFPIYMHP75K73PZ6/
> > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > ---
> > fs/ceph/addr.c | 1 -
> > fs/ceph/super.c | 1 +
> > 2 files changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index aa25bffd4823..02722ac86d73 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -1774,7 +1774,6 @@ int ceph_mmap(struct file *file, struct vm_area_struct *vma)
> >
> > if (!mapping->a_ops->readpage)
> > return -ENOEXEC;
> > - file_accessed(file);
> > vma->vm_ops = &ceph_vmops;
> > return 0;
> > }
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index e6987d295079..b73b4f75462c 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -1119,6 +1119,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
> > s->s_time_gran = 1;
> > s->s_time_min = 0;
> > s->s_time_max = U32_MAX;
> > + s->s_flags |= SB_NODIRATIME | SB_NOATIME;
> >
> > ret = set_anon_super_fc(s, fc);
> > if (ret != 0)
>
> (cc'ing Greg since he claimed this...)
Hmm? I don't think I've been in any atime discussions in years...
>
> I confess, I've never dug into the MDS code that should track atime, but
> I'm rather surprised that the MDS just drops those updates onto the
> floor.
>
> It's obviously updated when the mtime changes. The SETATTR operation
> allows the client to set the atime directly, and there is an "atime"
> slot in the cap structure that does get populated by the client. I guess
> though that it has never been 100% clear what cap the atime should be
> governed by so maybe it just always ignores that field?
>
> Anyway, I've no firm objection to this since no one in their right mind
> should use the atime anyway, but you may see some complaints if you just
> turn it off like this. There are some applications that use it.
> Hopefully no one is running those on ceph.
>
> It would be nice to document this somewhere as well -- maybe on the ceph
> POSIX conformance page?
>
> https://docs.ceph.com/en/latest/cephfs/posix/
>
> --
> Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] ceph: disable updating the atime since cephfs won't maintain it
2022-04-20 14:08 ` Gregory Farnum
@ 2022-04-21 1:12 ` Xiubo Li
0 siblings, 0 replies; 7+ messages in thread
From: Xiubo Li @ 2022-04-21 1:12 UTC (permalink / raw)
To: Gregory Farnum, Jeff Layton; +Cc: Ilya Dryomov, Venky Shankar, ceph-devel
On 4/20/22 10:08 PM, Gregory Farnum wrote:
> On Wed, Apr 20, 2022 at 6:57 AM Jeff Layton <jlayton@kernel.org> wrote:
>> On Wed, 2022-04-20 at 13:24 +0800, Xiubo Li wrote:
>>> Since the cephFS makes no attempt to maintain atime, we shouldn't
>>> try to update it in mmap and generic read cases and ignore updating
>>> it in direct and sync read cases.
>>>
>>> And even we update it in mmap and generic read cases we will drop
>>> it and won't sync it to MDS. And we are seeing the atime will be
>>> updated and then dropped to the floor again and again.
>>>
>>> URL: https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/VSJM7T4CS5TDRFF6XFPIYMHP75K73PZ6/
>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>> ---
>>> fs/ceph/addr.c | 1 -
>>> fs/ceph/super.c | 1 +
>>> 2 files changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>>> index aa25bffd4823..02722ac86d73 100644
>>> --- a/fs/ceph/addr.c
>>> +++ b/fs/ceph/addr.c
>>> @@ -1774,7 +1774,6 @@ int ceph_mmap(struct file *file, struct vm_area_struct *vma)
>>>
>>> if (!mapping->a_ops->readpage)
>>> return -ENOEXEC;
>>> - file_accessed(file);
>>> vma->vm_ops = &ceph_vmops;
>>> return 0;
>>> }
>>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>>> index e6987d295079..b73b4f75462c 100644
>>> --- a/fs/ceph/super.c
>>> +++ b/fs/ceph/super.c
>>> @@ -1119,6 +1119,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
>>> s->s_time_gran = 1;
>>> s->s_time_min = 0;
>>> s->s_time_max = U32_MAX;
>>> + s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>>>
>>> ret = set_anon_super_fc(s, fc);
>>> if (ret != 0)
>> (cc'ing Greg since he claimed this...)
> Hmm? I don't think I've been in any atime discussions in years...
Yeah, it's around 3 years ago in that link :-)
I checked the history of both libcephfs and kernel about the atime
changes, it not ever maintained.
-- Xiubo
>> I confess, I've never dug into the MDS code that should track atime, but
>> I'm rather surprised that the MDS just drops those updates onto the
>> floor.
>>
>> It's obviously updated when the mtime changes. The SETATTR operation
>> allows the client to set the atime directly, and there is an "atime"
>> slot in the cap structure that does get populated by the client. I guess
>> though that it has never been 100% clear what cap the atime should be
>> governed by so maybe it just always ignores that field?
>>
>> Anyway, I've no firm objection to this since no one in their right mind
>> should use the atime anyway, but you may see some complaints if you just
>> turn it off like this. There are some applications that use it.
>> Hopefully no one is running those on ceph.
>>
>> It would be nice to document this somewhere as well -- maybe on the ceph
>> POSIX conformance page?
>>
>> https://docs.ceph.com/en/latest/cephfs/posix/
>>
>> --
>> Jeff Layton <jlayton@kernel.org>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] ceph: disable updating the atime since cephfs won't maintain it
2022-04-20 13:56 ` Jeff Layton
2022-04-20 14:08 ` Gregory Farnum
@ 2022-04-21 1:08 ` Xiubo Li
2022-04-21 7:18 ` Xiubo Li
2 siblings, 0 replies; 7+ messages in thread
From: Xiubo Li @ 2022-04-21 1:08 UTC (permalink / raw)
To: Jeff Layton; +Cc: idryomov, vshankar, ceph-devel, Gregory Farnum
On 4/20/22 9:56 PM, Jeff Layton wrote:
> On Wed, 2022-04-20 at 13:24 +0800, Xiubo Li wrote:
>> Since the cephFS makes no attempt to maintain atime, we shouldn't
>> try to update it in mmap and generic read cases and ignore updating
>> it in direct and sync read cases.
>>
>> And even we update it in mmap and generic read cases we will drop
>> it and won't sync it to MDS. And we are seeing the atime will be
>> updated and then dropped to the floor again and again.
>>
>> URL: https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/VSJM7T4CS5TDRFF6XFPIYMHP75K73PZ6/
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>> fs/ceph/addr.c | 1 -
>> fs/ceph/super.c | 1 +
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>> index aa25bffd4823..02722ac86d73 100644
>> --- a/fs/ceph/addr.c
>> +++ b/fs/ceph/addr.c
>> @@ -1774,7 +1774,6 @@ int ceph_mmap(struct file *file, struct vm_area_struct *vma)
>>
>> if (!mapping->a_ops->readpage)
>> return -ENOEXEC;
>> - file_accessed(file);
>> vma->vm_ops = &ceph_vmops;
>> return 0;
>> }
>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>> index e6987d295079..b73b4f75462c 100644
>> --- a/fs/ceph/super.c
>> +++ b/fs/ceph/super.c
>> @@ -1119,6 +1119,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
>> s->s_time_gran = 1;
>> s->s_time_min = 0;
>> s->s_time_max = U32_MAX;
>> + s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>>
>> ret = set_anon_super_fc(s, fc);
>> if (ret != 0)
> (cc'ing Greg since he claimed this...)
>
> I confess, I've never dug into the MDS code that should track atime, but
> I'm rather surprised that the MDS just drops those updates onto the
> floor.
Yeah, it is. But that's because the client doesn't correctly handle it
and it must dirty the Fx caps:
3706 if ((dirty & CEPH_CAP_FILE_EXCL) && atime != pi->atime) {
3707 dout(7) << " atime " << pi->atime << " -> " << atime
3708 << " for " << *in << dendl;
3709 pi->atime = atime;
3710 }
I think the MDS code is only handling the setattr with Fx caps issued
case, which in client will buffer the atime changing and then flush it
back to MDS via the cap update requests.
Except the setattr in MDS there has no other place is handling the atime.
In libcephfs it never handle the atime in any case. While the in kclient
the vfs will do that.
>
> It's obviously updated when the mtime changes. The SETATTR operation
> allows the client to set the atime directly, and there is an "atime"
> slot in the cap structure that does get populated by the client. I guess
> though that it has never been 100% clear what cap the atime should be
> governed by so maybe it just always ignores that field?
IMO if we want to maintain the atime we need to check and dirty the Frc
caps instead when reading from a file in none setattr case.
Currently since we don't maintain the atime, the setattr just assumes
it's a metadata attribute and will be simply handled by the Fsx caps. I
think in setattr will allow to set the atime just used for backup use case.
> Anyway, I've no firm objection to this since no one in their right mind
> should use the atime anyway, but you may see some complaints if you just
> turn it off like this. There are some applications that use it.
> Hopefully no one is running those on ceph.
Partially supporting and incorrectly handling the atime are confusing,
usually assuming as a bug.
Before we thoroughly supporting it, maybe in future when needed, let's
disable it for now.
> It would be nice to document this somewhere as well -- maybe on the ceph
> POSIX conformance page?
>
> https://docs.ceph.com/en/latest/cephfs/posix/
Yeah, let's document here.
-- Xiubo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC PATCH] ceph: disable updating the atime since cephfs won't maintain it
2022-04-20 13:56 ` Jeff Layton
2022-04-20 14:08 ` Gregory Farnum
2022-04-21 1:08 ` Xiubo Li
@ 2022-04-21 7:18 ` Xiubo Li
2 siblings, 0 replies; 7+ messages in thread
From: Xiubo Li @ 2022-04-21 7:18 UTC (permalink / raw)
To: Jeff Layton; +Cc: idryomov, vshankar, ceph-devel, Gregory Farnum
On 4/20/22 9:56 PM, Jeff Layton wrote:
> On Wed, 2022-04-20 at 13:24 +0800, Xiubo Li wrote:
>> Since the cephFS makes no attempt to maintain atime, we shouldn't
>> try to update it in mmap and generic read cases and ignore updating
>> it in direct and sync read cases.
>>
>> And even we update it in mmap and generic read cases we will drop
>> it and won't sync it to MDS. And we are seeing the atime will be
>> updated and then dropped to the floor again and again.
>>
>> URL: https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/VSJM7T4CS5TDRFF6XFPIYMHP75K73PZ6/
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>> fs/ceph/addr.c | 1 -
>> fs/ceph/super.c | 1 +
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>> index aa25bffd4823..02722ac86d73 100644
>> --- a/fs/ceph/addr.c
>> +++ b/fs/ceph/addr.c
>> @@ -1774,7 +1774,6 @@ int ceph_mmap(struct file *file, struct vm_area_struct *vma)
>>
>> if (!mapping->a_ops->readpage)
>> return -ENOEXEC;
>> - file_accessed(file);
>> vma->vm_ops = &ceph_vmops;
>> return 0;
>> }
>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>> index e6987d295079..b73b4f75462c 100644
>> --- a/fs/ceph/super.c
>> +++ b/fs/ceph/super.c
>> @@ -1119,6 +1119,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
>> s->s_time_gran = 1;
>> s->s_time_min = 0;
>> s->s_time_max = U32_MAX;
>> + s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>>
>> ret = set_anon_super_fc(s, fc);
>> if (ret != 0)
> (cc'ing Greg since he claimed this...)
>
> I confess, I've never dug into the MDS code that should track atime, but
> I'm rather surprised that the MDS just drops those updates onto the
> floor.
>
> It's obviously updated when the mtime changes. The SETATTR operation
> allows the client to set the atime directly, and there is an "atime"
> slot in the cap structure that does get populated by the client. I guess
> though that it has never been 100% clear what cap the atime should be
> governed by so maybe it just always ignores that field?
>
> Anyway, I've no firm objection to this since no one in their right mind
> should use the atime anyway, but you may see some complaints if you just
> turn it off like this. There are some applications that use it.
> Hopefully no one is running those on ceph.
>
> It would be nice to document this somewhere as well -- maybe on the ceph
> POSIX conformance page?
>
> https://docs.ceph.com/en/latest/cephfs/posix/
Have update this in ceph PR https://github.com/ceph/ceph/pull/45979.
-- Xiubo
>
^ permalink raw reply [flat|nested] 7+ messages in thread