[19/20] pstore: fs superblock limits
diff mbox series

Message ID 20190730014924.2193-20-deepa.kernel@gmail.com
State New
Headers show
Series
  • vfs: Add support for timestamp limits
Related show

Commit Message

Deepa Dinamani July 30, 2019, 1:49 a.m. UTC
Also update the gran since pstore has microsecond granularity.

Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: anton@enomsg.org
Cc: ccross@android.com
Cc: keescook@chromium.org
Cc: tony.luck@intel.com
---
 fs/pstore/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Kees Cook July 30, 2019, 4:31 a.m. UTC | #1
On Mon, Jul 29, 2019 at 06:49:23PM -0700, Deepa Dinamani wrote:
> Also update the gran since pstore has microsecond granularity.

So, I'm fine with this, but technically the granularity depends on the
backend storage... many have no actual time keeping, though. My point is,
pstore's timestamps are really mostly a lie, but the most common backend
(ramoops) is seconds-granularity.

So, I'm fine with this, but it's a lie but it's a lie that doesn't
matter, so ...

Acked-by: Kees Cook <keescook@chromium.org>

I'm open to suggestions to improve it...

-Kees

> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> Cc: anton@enomsg.org
> Cc: ccross@android.com
> Cc: keescook@chromium.org
> Cc: tony.luck@intel.com
> ---
>  fs/pstore/inode.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 89a80b568a17..ee752f9fda57 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -388,7 +388,9 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
>  	sb->s_blocksize_bits	= PAGE_SHIFT;
>  	sb->s_magic		= PSTOREFS_MAGIC;
>  	sb->s_op		= &pstore_ops;
> -	sb->s_time_gran		= 1;
> +	sb->s_time_gran         = NSEC_PER_USEC;
> +	sb->s_time_min		= S64_MIN;
> +	sb->s_time_max		= S64_MAX;
>  
>  	parse_options(data);
>  
> -- 
> 2.17.1
>
Arnd Bergmann July 30, 2019, 7:36 a.m. UTC | #2
On Tue, Jul 30, 2019 at 6:31 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Jul 29, 2019 at 06:49:23PM -0700, Deepa Dinamani wrote:
> > Also update the gran since pstore has microsecond granularity.
>
> So, I'm fine with this, but technically the granularity depends on the
> backend storage... many have no actual time keeping, though. My point is,
> pstore's timestamps are really mostly a lie, but the most common backend
> (ramoops) is seconds-granularity.
>
> So, I'm fine with this, but it's a lie but it's a lie that doesn't
> matter, so ...
>
> Acked-by: Kees Cook <keescook@chromium.org>
>
> I'm open to suggestions to improve it...

If we don't care about using sub-second granularity, then setting it
to one second unconditionally here will make it always use that and
report it correctly.

       Arnd
Deepa Dinamani Aug. 2, 2019, 2:26 a.m. UTC | #3
On Tue, Jul 30, 2019 at 12:36 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Jul 30, 2019 at 6:31 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Jul 29, 2019 at 06:49:23PM -0700, Deepa Dinamani wrote:
> > > Also update the gran since pstore has microsecond granularity.
> >
> > So, I'm fine with this, but technically the granularity depends on the
> > backend storage... many have no actual time keeping, though. My point is,
> > pstore's timestamps are really mostly a lie, but the most common backend
> > (ramoops) is seconds-granularity.
> >
> > So, I'm fine with this, but it's a lie but it's a lie that doesn't
> > matter, so ...
> >
> > Acked-by: Kees Cook <keescook@chromium.org>
> >
> > I'm open to suggestions to improve it...
>
> If we don't care about using sub-second granularity, then setting it
> to one second unconditionally here will make it always use that and
> report it correctly.

Should this printf in ramoops_write_kmsg_hdr() also be fixed then?

        RAMOOPS_KERNMSG_HDR "%lld.%06lu-%c\n",
        (time64_t)record->time.tv_sec,
        record->time.tv_nsec / 1000,
        record->compressed ? 'C' : 'D');
    persistent_ram_write(prz, hdr, len);

ramoops_read_kmsg_hdr() doesn't read this as microseconds. Seems like
a mismatch from above.

If we want to agree that we just want seconds granularity for pstore,
we could replace the tv_nsec part to be all 0's if anybody else is
depending on this format.
I could drop this patch from the series and post that patch seperately.

Thanks,
-Deepa
Arnd Bergmann Aug. 2, 2019, 7:15 a.m. UTC | #4
On Fri, Aug 2, 2019 at 4:26 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> On Tue, Jul 30, 2019 at 12:36 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Jul 30, 2019 at 6:31 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Mon, Jul 29, 2019 at 06:49:23PM -0700, Deepa Dinamani wrote:
> > > > Also update the gran since pstore has microsecond granularity.
> > >
> > > So, I'm fine with this, but technically the granularity depends on the
> > > backend storage... many have no actual time keeping, though. My point is,
> > > pstore's timestamps are really mostly a lie, but the most common backend
> > > (ramoops) is seconds-granularity.
> > >
> > > So, I'm fine with this, but it's a lie but it's a lie that doesn't
> > > matter, so ...
> > >
> > > Acked-by: Kees Cook <keescook@chromium.org>
> > >
> > > I'm open to suggestions to improve it...
> >
> > If we don't care about using sub-second granularity, then setting it
> > to one second unconditionally here will make it always use that and
> > report it correctly.
>
> Should this printf in ramoops_write_kmsg_hdr() also be fixed then?
>
>         RAMOOPS_KERNMSG_HDR "%lld.%06lu-%c\n",
>         (time64_t)record->time.tv_sec,
>         record->time.tv_nsec / 1000,
>         record->compressed ? 'C' : 'D');
>     persistent_ram_write(prz, hdr, len);
>
> ramoops_read_kmsg_hdr() doesn't read this as microseconds. Seems like
> a mismatch from above.

Good catch. This seems to go back to commit 3f8f80f0cfeb ("pstore/ram:
Read and write to the 'compressed' flag of pstore"), which introduced the
nanosecond read. The write function however has always used
microseconds, and that was kept when the implementation changed
from timeval to timespec in commit 1e817fb62cd1 ("time: create
__getnstimeofday for WARNless calls").

> If we want to agree that we just want seconds granularity for pstore,
> we could replace the tv_nsec part to be all 0's if anybody else is
> depending on this format.
> I could drop this patch from the series and post that patch seperately.

We should definitely fix it to not produce a bogus nanosecond value.
Whether using full seconds or microsecond resolution is better here,
I don't know. It seems that pstore records generally get created
with a nanosecond nanosecond accurate timestamp from
ktime_get_real_fast_ns() and then truncated to the resolution of the
backend, rather than the normal jiffies-accurate inode timestamps that
we have for regular file systems.

This might mean that we do want the highest possible resolution
and not further truncate here, in case that information ends
up being useful afterwards.

         Arnd
Deepa Dinamani Aug. 18, 2019, 2 p.m. UTC | #5
On Fri, Aug 2, 2019 at 12:15 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Aug 2, 2019 at 4:26 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> >
> > On Tue, Jul 30, 2019 at 12:36 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Tue, Jul 30, 2019 at 6:31 AM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > On Mon, Jul 29, 2019 at 06:49:23PM -0700, Deepa Dinamani wrote:
> > > > > Also update the gran since pstore has microsecond granularity.
> > > >
> > > > So, I'm fine with this, but technically the granularity depends on the
> > > > backend storage... many have no actual time keeping, though. My point is,
> > > > pstore's timestamps are really mostly a lie, but the most common backend
> > > > (ramoops) is seconds-granularity.
> > > >
> > > > So, I'm fine with this, but it's a lie but it's a lie that doesn't
> > > > matter, so ...
> > > >
> > > > Acked-by: Kees Cook <keescook@chromium.org>
> > > >
> > > > I'm open to suggestions to improve it...
> > >
> > > If we don't care about using sub-second granularity, then setting it
> > > to one second unconditionally here will make it always use that and
> > > report it correctly.
> >
> > Should this printf in ramoops_write_kmsg_hdr() also be fixed then?
> >
> >         RAMOOPS_KERNMSG_HDR "%lld.%06lu-%c\n",
> >         (time64_t)record->time.tv_sec,
> >         record->time.tv_nsec / 1000,
> >         record->compressed ? 'C' : 'D');
> >     persistent_ram_write(prz, hdr, len);
> >
> > ramoops_read_kmsg_hdr() doesn't read this as microseconds. Seems like
> > a mismatch from above.
>
> Good catch. This seems to go back to commit 3f8f80f0cfeb ("pstore/ram:
> Read and write to the 'compressed' flag of pstore"), which introduced the
> nanosecond read. The write function however has always used
> microseconds, and that was kept when the implementation changed
> from timeval to timespec in commit 1e817fb62cd1 ("time: create
> __getnstimeofday for WARNless calls").
>
> > If we want to agree that we just want seconds granularity for pstore,
> > we could replace the tv_nsec part to be all 0's if anybody else is
> > depending on this format.
> > I could drop this patch from the series and post that patch seperately.
>
> We should definitely fix it to not produce a bogus nanosecond value.
> Whether using full seconds or microsecond resolution is better here,
> I don't know. It seems that pstore records generally get created
> with a nanosecond nanosecond accurate timestamp from
> ktime_get_real_fast_ns() and then truncated to the resolution of the
> backend, rather than the normal jiffies-accurate inode timestamps that
> we have for regular file systems.
>
> This might mean that we do want the highest possible resolution
> and not further truncate here, in case that information ends
> up being useful afterwards.

I made a list of granularities used by pstore drivers using pstore_register():

1. efi - seconds
2. ramoops - microsecond
3. erst - seconds
4. powerpc/nvram64 - seconds

I will leave pstore granularity at nanoseconds and fix the ramoops read.

-Deepa

Patch
diff mbox series

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 89a80b568a17..ee752f9fda57 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -388,7 +388,9 @@  static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_blocksize_bits	= PAGE_SHIFT;
 	sb->s_magic		= PSTOREFS_MAGIC;
 	sb->s_op		= &pstore_ops;
-	sb->s_time_gran		= 1;
+	sb->s_time_gran         = NSEC_PER_USEC;
+	sb->s_time_min		= S64_MIN;
+	sb->s_time_max		= S64_MAX;
 
 	parse_options(data);