Re: [PATCH 19/20] pstore: fs superblock limits

[Deepa Dinamani]

On Fri, Aug 2, 2019 at 12:15 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Fri, Aug 2, 2019 at 4:26 AM Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
> >
> > On Tue, Jul 30, 2019 at 12:36 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > >
> > > On Tue, Jul 30, 2019 at 6:31 AM Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > >
> > > > 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