search

cxl-micron-reskit / famfs

This is the user space repo for famfs, the fabric-attached memory file system
Apache License 2.0
31 stars 9 forks source link

fix invalidate_processor_cache length to match mmap() len #67

Closed leeq2016 closed 2 months ago

leeq2016 commented 3 months ago

The old memory data currently used may be an illegal value, causing famfs to fail to initialize correctly (possibly causing a segfault).

jagalactic commented 3 months ago

I need to think a bit about this; Looking through the superblock and log lengths, the handling is a bit inconsistent sometimes using the superblock fields and sometimes using the #defines. The intent is:

  1. Superblock length is fixed at 2MiB, so we always know how to map that
  2. Primary log length is variable field in the superblock, but always a multiple of 2MiB (but currently we have only tested with the default log length of 8MiB)

Currently there are log_length fields in both the superblock and the log header; having the log length in the superblock is convenient because it always needs to be verified before the log can be mapped - and this can handle a non-invariant log length.

Unless I'm missing something, this change is basically a NOP except that it won't handle a different log length if specified in the superblock.

leeq2016 commented 3 months ago

I think there are two reasons for the modification of this PR:

  1. When initialized for the first time, the length field in the memory pointed to by pmem may be a random value? Flush the cache based on this length does not meet expectations.
  2. After changing the log size of famfs, the cache cannot be flushed as expected. (When the log size becomes larger, part of the cache will not be cleaned. When the log size becomes smaller, the mkfs tool will always core dump, because it accesses a virtual address range outside mmap.)

Regards! :)

jagalactic commented 3 months ago

The branch/PR I'm working on will be consistent as to how it references superblock and log lengths...

jagalactic commented 2 months ago

Please take a look at https://github.com/cxl-micron-reskit/famfs/pull/74. It generalizes the configurable log length, and I think it fixes the cache related issues.

It also fixes some buggy code related to FAMFS_ALLOCATION_UNIT, although it does not make that a configurable/superblock parameter (that can happen soon).

leeq2016 commented 2 months ago

Close this PR, this issue will be fixed in #74