Re: [PATCH] exec: fetch the alignment of Linux devdax pmem character device nodes

[Joao Martins]

On 4/8/20 3:25 AM, Liu, Jingqi wrote:
> On 4/8/2020 2:28 AM, Joao Martins wrote:
>> On 4/7/20 5:55 PM, Dan Williams wrote:
>>> On Tue, Apr 7, 2020 at 4:01 AM Joao Martins <address@hidden> wrote:
>>>> On 4/1/20 4:13 AM, Jingqi Liu wrote:
>>>>> If the backend file is devdax pmem character device, the alignment
>>>>> specified by the option 'align=NUM' in the '-object memory-backend-file'
>>>>> needs to match the alignment requirement of the devdax pmem character 
>>>>> device.
>>>>>
>>>>> This patch fetches the devdax pmem file 'align', so that we can compare
>>>>> it with the NUM of 'align=NUM'.
>>>>> The NUM needs to be larger than or equal to the devdax pmem file 'align'.
>>>>>
>>>>> It also fixes the problem that mmap() returns failure in qemu_ram_mmap()
>>>>> when the NUM of 'align=NUM' is less than the devdax pmem file 'align'.
>>>>>
>>>>> Cc: Dan Williams <address@hidden>
>>>>> Signed-off-by: Jingqi Liu <address@hidden>
>>>>> ---
>>>>>   exec.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/exec.c b/exec.c
>>>>> index de9d949902..8221abffec 100644
>>>>> --- a/exec.c
>>>>> +++ b/exec.c
>>>>> @@ -1736,6 +1736,42 @@ static int64_t get_file_size(int fd)
>>>>>       return size;
>>>>>   }
>>>>>
>>>>> +static int64_t get_file_align(int fd)
>>>>> +{
>>>>> +    int64_t align = -1;
>>>>> +#if defined(__linux__)
>>>>> +    struct stat st;
>>>>> +
>>>>> +    if (fstat(fd, &st) < 0) {
>>>>> +        return -errno;
>>>>> +    }
>>>>> +
>>>>> +    /* Special handling for devdax character devices */
>>>>> +    if (S_ISCHR(st.st_mode)) {
>>>>> +        g_autofree char *subsystem_path = NULL;
>>>>> +        g_autofree char *subsystem = NULL;
>>>>> +
>>>>> +        subsystem_path = g_strdup_printf("/sys/dev/char/%d:%d/subsystem",
>>>>> +                                         major(st.st_rdev), 
>>>>> minor(st.st_rdev));
>>>>> +        subsystem = g_file_read_link(subsystem_path, NULL);
>>>>> +
>>>>> +        if (subsystem && g_str_has_suffix(subsystem, "/dax")) {
>>>>> +            g_autofree char *align_path = NULL;
>>>>> +            g_autofree char *align_str = NULL;
>>>>> +
>>>>> +            align_path = 
>>>>> g_strdup_printf("/sys/dev/char/%d:%d/device/align",
>>>>> +                                    major(st.st_rdev), 
>>>>> minor(st.st_rdev));
>>>>> +
>>>> Perhaps, you meant instead:
>>>>
>>>>          /sys/dev/char/%d:%d/align
>>>>
>>> Hmm, are you sure that's working?
>> It is, except that I made the slight mistake of testing with a bunch of wip
>> patches on top which one of them actually adds the 'align' to child dax 
>> device.
>>
>> Argh, my apologies - and thanks for noticing.
>>
>>> I expect the alignment to be found
>>> in the region device:
>>>
>>> /sys/class/dax:
>>> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0012:00/ndbus1/region1/dax1.1/dax1.0
>>> $(readlink -f /sys/dev/char/253\:263)/../align
>>> $(readlink -f /sys/dev/char/253\:263)/device/align
>>>
>>>
>>> /sys/bus/dax:
>>> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0012:00/ndbus1/region1/dax1.0/dax1.0
>>> $(readlink -f /sys/dev/char/253\:265)/../align
>>> $(readlink -f /sys/dev/char/253\:265)/device/align <-- No such file
>>>
>>> The use of the /sys/dev/char/%d:%d/device is only supported by the
>>> deprecated /sys/class/dax.
> 
> Hi Dan,
> 
> Thanks for your comments.
> 
> Seems it is a mistake.
> 
> It should be: $(readlink -f /sys/dev/char/253\:263)/../../align
> 

Hmm, perhaps you have an extra '../' in the path? This works for me:

# ls $(readlink -f /sys/dev/char/252\:0/../align)
/sys/devices/platform/e820_pmem/ndbus0/region0/dax0.0/dax0.0/../align
# cat $(readlink -f /sys/dev/char/252\:0)/../align
2097152
# cat /sys/dev/char/252\:0/../align
2097152

>> I don't have the deprecated dax class enabled as could you tell, so the 
>> second
>> case is what I was testing. Except it wasn't a namespace/nvdimm but rather an
>> hmem device-dax.
>>
>> '../align' though covers only one case? What about hmem which '../align' 
>> returns
>> ENOENT; perhaps using '../dax_region/align' instead which is common to both?
>> Albeit that wouldn't address the sub-division devices (that I mention above)
> 
> Seems that you mean to use $(readlink -f 
> /sys/dev/char/253\:263)/../../dax_region/align.
> 
> Right ?
> 

An extra '../' ?

# ls $(readlink -f /sys/dev/char/252\:0/../dax_region/align)
/sys/devices/platform/e820_pmem/ndbus0/region0/dax0.0/dax0.0/../align
# cat $(readlink -f /sys/dev/char/252\:0)/../dax_region/align
2097152
# cat /sys/dev/char/252\:0/../dax_region/align
2097152

For HMAT/hmem devdax, though, only 'dax_region/align' is available for now:

# ls $(readlink -f /sys/dev/char/252:0)/../align
ls: cannot access /sys/devices/platform/hmem.0/dax0.0/../align: No such file or
directory
# ls $(readlink -f /sys/dev/char/252:0)/../dax_region/align
/sys/devices/platform/hmem.0/dax0.0/../dax_region/align
# cat $(readlink -f /sys/dev/char/252:0)/../dax_region/align
2097152

The 'dax_region/align' was just an idea mainly because it's common to both
device-dax devices -- not sure how others feel about it.

        Joao

> Thanks,
> 
> Jingqi
> 
>>> The current /sys/bus/dax device-model can
>>> be a drop in replacement as long as software is not written to the
>>> /sys/class sysfs layout, i.e. it uses ../ instead of device/ to walk
>>> to the region properties.
>>>
>> /nods