lkml.org 
[lkml]   [2021]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 06/11] mm, pmem: Implement ->memory_failure() in pmem driver
> +static int pmem_pagemap_memory_failure(struct dev_pagemap *pgmap,
> + unsigned long pfn, int flags)
> +{
> + struct pmem_device *pdev;
> + struct gendisk *disk;
> + loff_t disk_offset;
> + int rc = 0;
> + unsigned long size = page_size(pfn_to_page(pfn));
> +
> + pdev = container_of(pgmap, struct pmem_device, pgmap);
> + disk = pdev->disk;

Would be nice to initialize this at the time of declaration:

struct pmem_device *pdev =
container_of(pgmap, struct pmem_device, pgmap);
struct gendisk *disk = pdev->disk
unsigned long size = page_size(pfn_to_page(pfn));

> + if (!disk)
> + return -ENXIO;
> +
> + disk_offset = PFN_PHYS(pfn) - pdev->phys_addr - pdev->data_offset;
> + if (disk->fops->corrupted_range) {
> + rc = disk->fops->corrupted_range(disk, NULL, disk_offset,
> + size, &flags);
> + if (rc == -ENODEV)
> + rc = -ENXIO;
> + } else
> + rc = -EOPNOTSUPP;

Why do we need the disk and fops check here? A pgmap registered by pmem.c
should always have a disk with pmem_fops. And more importantly this
has no business going through the block layer.

Instead the file system should deposit a callback when starting to use
the dax_device using fs_dax_get_by_bdev / dax_get_by_host and a private
data (the superblock), and we avoid all the lookup problems.

> +int mf_generic_kill_procs(unsigned long long pfn, int flags)

This function seems to be only used inside of memory-failure.c, so it
could be marked static. Also I'd name it dax_generic_memory_failure
or something like that to match the naming of the ->memory_failure
pgmap operation.

Also maybe just splitting this out into a helper would be a nice prep
patch.

\
 
 \ /
  Last update: 2021-02-10 14:42    [W:0.226 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site