Re: [PATCH 19/26] block: move the nowait flag to queue_limits
On 6/17/24 15:04, Christoph Hellwig wrote: > Move the nowait flag into the queue_limits feature field so that it can > be set atomically with the queue frozen. > > Stacking drivers are simplified in that they now can simply set the > flag, and blk_stack_limits will clear it when the features is not > supported by any of the underlying devices. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 16/26] block: move the io_stat flag setting to queue_limits
On 6/17/24 15:04, Christoph Hellwig wrote: > Move the io_stat flag into the queue_limits feature field so that it can > be set atomically with the queue frozen. > > Simplify md and dm to set the flag unconditionally instead of avoiding > setting a simple flag for cases where it already is set by other means, > which is a bit pointless. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 13/26] block: move cache control settings out of queue->flags
On 6/17/24 15:04, Christoph Hellwig wrote: > Move the cache control settings into the queue_limits so that the flags > can be set atomically with the device queue frozen. > > Add new features and flags field for the driver set flags, and internal > (usually sysfs-controlled) flags in the block layer. Note that we'll > eventually remove enough field from queue_limits to bring it back to the > previous size. > > The disable flag is inverted compared to the previous meaning, which > means it now survives a rescan, similar to the max_sectors and > max_discard_sectors user limits. > > The FLUSH and FUA flags are now inherited by blk_stack_limits, which > simplified the code in dm a lot, but also causes a slight behavior > change in that dm-switch and dm-unstripe now advertise a write cache > despite setting num_flush_bios to 0. The I/O path will handle this > gracefully, but as far as I can tell the lack of num_flush_bios > and thus flush support is a pre-existing data integrity bug in those > targets that really needs fixing, after which a non-zero num_flush_bios > should be required in dm for targets that map to underlying devices. > > Signed-off-by: Christoph Hellwig > Acked-by: Ulf Hansson [mmc] A few nits below. With these fixed, Reviewed-by: Damien Le Moal > +Implementation details for bio based block drivers > +-- > + > +For bio based drivers the REQ_PREFLUSH and REQ_FUA bit are simplify passed on ...bit are simplify... -> ...bits are simply... > +to the driver if the drivers sets the BLK_FEAT_WRITE_CACHE flag and the > drivers > +needs to handle them. s/drivers/driver (2 times) > -and the driver must handle write requests that have the REQ_FUA bit set > -in prep_fn/request_fn. If the FUA bit is not natively supported the block > -layer turns it into an empty REQ_OP_FLUSH request after the actual write. > +When the BLK_FEAT_FUA flags is set, the REQ_FUA bit simplify passed on for > the s/bit simplify/bit is simply -- Damien Le Moal Western Digital Research
Re: [PATCH 07/26] loop: also use the default block size from an underlying block device
On 6/17/24 15:04, Christoph Hellwig wrote: > Fix the code in loop_reconfigure_limits to pick a default block size for > O_DIRECT file descriptors to also work when the loop device sits on top > of a block device and not just on a regular file on a block device based > file system. > > Signed-off-by: Christoph Hellwig > Reviewed-by: Hannes Reinecke > Reviewed-by: Bart Van Assche Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 06/26] loop: regularize upgrading the block size for direct I/O
On 6/17/24 15:04, Christoph Hellwig wrote: > The LOOP_CONFIGURE path automatically upgrades the block size to that > of the underlying file for O_DIRECT file descriptors, but the > LOOP_SET_BLOCK_SIZE path does not. Fix this by lifting the code to > pick the block size into common code. > > Signed-off-by: Christoph Hellwig > Reviewed-by: Hannes Reinecke > Reviewed-by: Bart Van Assche Looks good to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 03/26] sd: move zone limits setup out of sd_read_block_characteristics
On 6/17/24 15:04, Christoph Hellwig wrote: > Move a bit of code that sets up the zone flag and the write granularity > into sd_zbc_read_zones to be with the rest of the zoned limits. > > Signed-off-by: Christoph Hellwig Looks good. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 02/26] sd: move zone limits setup out of sd_read_block_characteristics
On 6/17/24 13:53, Christoph Hellwig wrote: > On Mon, Jun 17, 2024 at 08:01:04AM +0900, Damien Le Moal wrote: >> On 6/13/24 18:39, Christoph Hellwig wrote: >>> On Tue, Jun 11, 2024 at 02:51:24PM +0900, Damien Le Moal wrote: >>>>> + if (sdkp->device->type == TYPE_ZBC) >>>> >>>> Nit: use sd_is_zoned() here ? >>> >>> Actually - is there much in even keeping sd_is_zoned now that the >>> host aware support is removed? Just open coding the type check isn't >>> any more code, and probably easier to follow. >> >> Removing this helper is fine by me. > > FYI, I've removed it yesterday, but not done much of the cleanups suggest > here. We should probably do those in a follow up up, uncluding removing > the !ZBC check in sd_zbc_check_zoned_characteristics. OK. I will send that once your series in queued. -- Damien Le Moal Western Digital Research
Re: [PATCH 02/26] sd: move zone limits setup out of sd_read_block_characteristics
On 6/13/24 18:39, Christoph Hellwig wrote: > On Tue, Jun 11, 2024 at 02:51:24PM +0900, Damien Le Moal wrote: >>> + if (sdkp->device->type == TYPE_ZBC) >> >> Nit: use sd_is_zoned() here ? > > Actually - is there much in even keeping sd_is_zoned now that the > host aware support is removed? Just open coding the type check isn't > any more code, and probably easier to follow. Removing this helper is fine by me. There are only 2 call sites in sd.c and the some of 4 calls in sd_zbc.c are not really needed: 1) The call in sd_zbc_print_zones() is not needed at all since this function is called only for a zoned drive from sd_zbc_revalidate_zones(). 2) The calls in sd_zbc_report_zones() and sd_zbc_cmnd_checks() are probably useless as these are called only for zoned drives in the first place. The checks would be useful only for passthrough commands, but then we do not really care about these and the user will get a failure anyway if it tries to do ZBC commands on non-ZBC drives. 3) That leaves only the call in sd_zbc_read_zones() but that check can probably be moved to sd.c to conditionally call sd_zbc_read_zones(). -- Damien Le Moal Western Digital Research
Re: [PATCH 26/26] block: move the bounce flag into the feature field
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the bounce field into the flags field to reclaim a little bit of s/flags/feature > space. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 25/26] block: move the skip_tagset_quiesce flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the skip_tagset_quiesce flag into the queue_limits feature field so > that it can be set atomically and all I/O is frozen when changing the > flag. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 24/26] block: move the pci_p2pdma flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the pci_p2pdma flag into the queue_limits feature field so that it > can be set atomically and all I/O is frozen when changing the flag. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 23/26] block: move the zone_resetall flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the zone_resetall flag into the queue_limits feature field so that > it can be set atomically and all I/O is frozen when changing the flag. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 22/26] block: move the zoned flag into the feature field
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the boolean zoned field into the flags field to reclaim a little > bit of space. Nit: flags -> feature flags > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 21/26] block: move the poll flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the poll flag into the queue_limits feature field so that it > can be set atomically and all I/O is frozen when changing the flag. > > Stacking drivers are simplified in that they now can simply set the > flag, and blk_stack_limits will clear it when the features is not > supported by any of the underlying devices. > > Signed-off-by: Christoph Hellwig Kind of the same remark as for io_stat about this not really being a device feature. But I guess seeing "features" as a queue feature rather than just a device feature makes it OK to have poll (and io_stat) as a feature rather than a flag. So: Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 20/26] block: move the dax flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the dax flag into the queue_limits feature field so that it > can be set atomically and all I/O is frozen when changing the flag. > > Signed-off-by: Christoph Hellwig Looks good. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 19/26] block: move the nowait flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the nowait flag into the queue_limits feature field so that it > can be set atomically and all I/O is frozen when changing the flag. > > Stacking drivers are simplified in that they now can simply set the > flag, and blk_stack_limits will clear it when the features is not > supported by any of the underlying devices. > > Signed-off-by: Christoph Hellwig > @@ -1825,9 +1815,7 @@ int dm_table_set_restrictions(struct dm_table *t, > struct request_queue *q, > int r; > > if (dm_table_supports_nowait(t)) > - blk_queue_flag_set(QUEUE_FLAG_NOWAIT, q); > - else > - blk_queue_flag_clear(QUEUE_FLAG_NOWAIT, q); > + limits->features &= ~BLK_FEAT_NOWAIT; Shouldn't you set the flag here instead of clearing it ? -- Damien Le Moal Western Digital Research
Re: [PATCH 18/26] block: move the synchronous flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the synchronous flag into the queue_limits feature field so that it > can be set atomically and all I/O is frozen when changing the flag. > > Signed-off-by: Christoph Hellwig Looks good. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 17/26] block: move the stable_write flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the io_stat flag into the queue_limits feature field so that it can s/io_stat/stable_write > be set atomically and all I/O is frozen when changing the flag. > > The flag is now inherited by blk_stack_limits, which greatly simplifies > the code in dm, and fixed md which previously did not pass on the flag > set on lower devices. > > Signed-off-by: Christoph Hellwig Other than the nit above, looks OK to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 16/26] block: move the io_stat flag setting to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the io_stat flag into the queue_limits feature field so that it > can be set atomically and all I/O is frozen when changing the flag. Why a feature ? It seems more appropriate for io_stat to be a flag rather than a feature as that is a block layer thing rather than a device characteristic, no ? > > Simplify md and dm to set the flag unconditionally instead of avoiding > setting a simple flag for cases where it already is set by other means, > which is a bit pointless. > > Signed-off-by: Christoph Hellwig -- Damien Le Moal Western Digital Research
Re: [PATCH 15/26] block: move the add_random flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the add_random flag into the queue_limits feature field so that it > can be set atomically and all I/O is frozen when changing the flag. Same remark as the previous patches for the end of this sentence.c > > Note that this also removes code from dm to clear the flag based on > the underlying devices, which can't be reached as dm devices will > always start out without the flag set. > > Signed-off-by: Christoph Hellwig Other than that, looks OK to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 14/26] block: move the nonrot flag to queue_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move the norot flag into the queue_limits feature field so that it can be s/norot/nonrot > set atomically and all I/O is frozen when changing the flag. and... -> with the queue frozen when... ? > > Use the chance to switch to defaulting to non-rotational and require > the driver to opt into rotational, which matches the polarity of the > sysfs interface. > > For the z2ram, ps3vram, 2x memstick, ubiblock and dcssblk the new > rotational flag is not set as they clearly are not rotational despite > this being a behavior change. There are some other drivers that > unconditionally set the rotational flag to keep the existing behavior > as they arguably can be used on rotational devices even if that is > probably not their main use today (e.g. virtio_blk and drbd). > > The flag is automatically inherited in blk_stack_limits matching the > existing behavior in dm and md. > > Signed-off-by: Christoph Hellwig Other than that, looks good to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 13/26] block: move cache control settings out of queue->flags
gs |= BLK_FLAGS_WRITE_CACHE_DISABLED;
> + else
> + lim.flags &= ~BLK_FLAGS_WRITE_CACHE_DISABLED;
> + err = queue_limits_commit_update(q, );
> + if (err)
> + return err;
> return count;
> }
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index fd789eeb62d943..fbe125d55e25b4 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1686,34 +1686,16 @@ int dm_calculate_queue_limits(struct dm_table *t,
> return validate_hardware_logical_block_alignment(t, limits);
> }
>
> -static int device_flush_capable(struct dm_target *ti, struct dm_dev *dev,
> - sector_t start, sector_t len, void *data)
> -{
> - unsigned long flush = (unsigned long) data;
> - struct request_queue *q = bdev_get_queue(dev->bdev);
> -
> - return (q->queue_flags & flush);
> -}
> -
> -static bool dm_table_supports_flush(struct dm_table *t, unsigned long flush)
> +/*
> + * Check if an target requires flush support even if none of the underlying
s/an/a
> + * devices need it (e.g. to persist target-specific metadata).
> + */
> +static bool dm_table_supports_flush(struct dm_table *t)
> {
> - /*
> - * Require at least one underlying device to support flushes.
> - * t->devices includes internal dm devices such as mirror logs
> - * so we need to use iterate_devices here, which targets
> - * supporting flushes must provide.
> - */
> for (unsigned int i = 0; i < t->num_targets; i++) {
> struct dm_target *ti = dm_table_get_target(t, i);
>
> - if (!ti->num_flush_bios)
> - continue;
> -
> - if (ti->flush_supported)
> - return true;
> -
> - if (ti->type->iterate_devices &&
> - ti->type->iterate_devices(ti, device_flush_capable, (void
> *) flush))
> + if (ti->num_flush_bios && ti->flush_supported)
> return true;
> }
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index c792d4d81e5fcc..4e8931a2c76b07 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -282,6 +282,28 @@ static inline bool blk_op_is_passthrough(blk_opf_t op)
> return op == REQ_OP_DRV_IN || op == REQ_OP_DRV_OUT;
> }
>
> +/* flags set by the driver in queue_limits.features */
> +enum {
> + /* supports a a volatile write cache */
Repeated "a".
> + BLK_FEAT_WRITE_CACHE = (1u << 0),
> +
> + /* supports passing on the FUA bit */
> + BLK_FEAT_FUA= (1u << 1),
> +};
> +static inline bool blk_queue_write_cache(struct request_queue *q)
> +{
> + return (q->limits.features & BLK_FEAT_WRITE_CACHE) &&
> + (q->limits.flags & BLK_FLAGS_WRITE_CACHE_DISABLED);
Hmm, shouldn't this be !(q->limits.flags & BLK_FLAGS_WRITE_CACHE_DISABLED) ?
> +}
> +
> static inline bool bdev_write_cache(struct block_device *bdev)
> {
> - return test_bit(QUEUE_FLAG_WC, _get_queue(bdev)->queue_flags);
> + return blk_queue_write_cache(bdev_get_queue(bdev));
> }
>
> static inline bool bdev_fua(struct block_device *bdev)
> {
> - return test_bit(QUEUE_FLAG_FUA, _get_queue(bdev)->queue_flags);
> + return bdev_get_queue(bdev)->limits.features & BLK_FEAT_FUA;
> }
>
> static inline bool bdev_nowait(struct block_device *bdev)
--
Damien Le Moal
Western Digital Research
Re: [PATCH 12/26] block: remove blk_flush_policy
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Fold blk_flush_policy into the only caller to prepare for pending changes > to it. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 11/26] block: freeze the queue in queue_attr_store
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > queue_attr_store updates attributes used to control generating I/O, and > can cause malformed bios if changed with I/O in flight. Freeze the queue > in common code instead of adding it to almost every attribute. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 10/26] xen-blkfront: don't disable cache flushes when they fail
On 6/11/24 2:19 PM, Christoph Hellwig wrote:
> blkfront always had a robust negotiation protocol for detecting a write
> cache. Stop simply disabling cache flushes when they fail as that is
> a grave error.
>
> Signed-off-by: Christoph Hellwig
Looks good to me but maybe mention that removal of xlvbd_flush() as well ?
And it feels like the "stop disabling cache flushes when they fail" part should
be a fix patch sent separately...
Anyway, for both parts, feel free to add:
Reviewed-by: Damien Le Moal
> ---
> drivers/block/xen-blkfront.c | 29 +
> 1 file changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 9b4ec3e4908cce..9794ac2d3299d1 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -982,18 +982,6 @@ static const char *flush_info(struct blkfront_info *info)
> return "barrier or flush: disabled;";
> }
>
> -static void xlvbd_flush(struct blkfront_info *info)
> -{
> - blk_queue_write_cache(info->rq, info->feature_flush ? true : false,
> - info->feature_fua ? true : false);
> - pr_info("blkfront: %s: %s %s %s %s %s %s %s\n",
> - info->gd->disk_name, flush_info(info),
> - "persistent grants:", info->feature_persistent ?
> - "enabled;" : "disabled;", "indirect descriptors:",
> - info->max_indirect_segments ? "enabled;" : "disabled;",
> - "bounce buffer:", info->bounce ? "enabled" : "disabled;");
> -}
> -
> static int xen_translate_vdev(int vdevice, int *minor, unsigned int *offset)
> {
> int major;
> @@ -1162,7 +1150,15 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
> info->sector_size = sector_size;
> info->physical_sector_size = physical_sector_size;
>
> - xlvbd_flush(info);
> + blk_queue_write_cache(info->rq, info->feature_flush ? true : false,
> + info->feature_fua ? true : false);
> +
> + pr_info("blkfront: %s: %s %s %s %s %s %s %s\n",
> + info->gd->disk_name, flush_info(info),
> + "persistent grants:", info->feature_persistent ?
> + "enabled;" : "disabled;", "indirect descriptors:",
> + info->max_indirect_segments ? "enabled;" : "disabled;",
> + "bounce buffer:", info->bounce ? "enabled" : "disabled;");
>
> if (info->vdisk_info & VDISK_READONLY)
> set_disk_ro(gd, 1);
> @@ -1622,13 +1618,6 @@ static irqreturn_t blkif_interrupt(int irq, void
> *dev_id)
> info->gd->disk_name,
> op_name(bret.operation));
> blkif_req(req)->error = BLK_STS_NOTSUPP;
> }
> - if (unlikely(blkif_req(req)->error)) {
> - if (blkif_req(req)->error == BLK_STS_NOTSUPP)
> - blkif_req(req)->error = BLK_STS_OK;
> - info->feature_fua = 0;
> - info->feature_flush = 0;
> - xlvbd_flush(info);
> - }
> fallthrough;
> case BLKIF_OP_READ:
> case BLKIF_OP_WRITE:
--
Damien Le Moal
Western Digital Research
Re: [PATCH 09/26] nbd: move setting the cache control flags to __nbd_set_size
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Move setting the cache control flags in nbd in preparation for moving > these flags into the queue_limits structure. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 08/26] virtio_blk: remove virtblk_update_cache_mode
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > virtblk_update_cache_mode boils down to a single call to > blk_queue_write_cache. Remove it in preparation for moving the cache > control flags into the queue_limits. > > Signed-off-by: Christoph Hellwig Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 02/26] sd: move zone limits setup out of sd_read_block_characteristics
On 6/11/24 2:54 PM, Christoph Hellwig wrote:
> On Tue, Jun 11, 2024 at 07:52:39AM +0200, Christoph Hellwig wrote:
>>> Maybe we should clear the other zone related limits here ? If the drive is
>>> reformatted/converted from SMR to CMR (FORMAT WITH PRESET), the other zone
>>> limits may be set already, no ?
>>
>> blk_validate_zoned_limits already takes care of that.
>
> Sorry, brainfart. The integrity code does that, but not the zoned
> code. I suspect the core code might be a better place for it,
> though.
Yes. Just replied to your previous email before seeing this one.
I think that:
static int blk_validate_zoned_limits(struct queue_limits *lim)
{
if (!lim->zoned) {
if (WARN_ON_ONCE(lim->max_open_zones) ||
WARN_ON_ONCE(lim->max_active_zones) ||
WARN_ON_ONCE(lim->zone_write_granularity) ||
WARN_ON_ONCE(lim->max_zone_append_sectors))
return -EINVAL;
return 0;
}
...
could be changed into:
static int blk_validate_zoned_limits(struct queue_limits *lim)
{
if (!lim->zoned) {
lim->max_open_zones = 0;
lim->max_active_zones = 0;
lim->zone_write_granularity = 0;
lim->max_zone_append_sectors = 0
return 0;
}
But then we would not see "bad" drivers. Could have a small
blk_clear_zoned_limits(struct queue_limits *lim)
helper too.
--
Damien Le Moal
Western Digital Research
Re: [PATCH 02/26] sd: move zone limits setup out of sd_read_block_characteristics
On 6/11/24 2:52 PM, Christoph Hellwig wrote:
> On Tue, Jun 11, 2024 at 02:51:24PM +0900, Damien Le Moal wrote:
>>> - if (lim->zoned)
>>> + if (sdkp->device->type == TYPE_ZBC)
>>
>> Nit: use sd_is_zoned() here ?
>
> Yes.
>
>>> - if (!sd_is_zoned(sdkp))
>>> + if (!sd_is_zoned(sdkp)) {
>>> + lim->zoned = false;
>>
>> Maybe we should clear the other zone related limits here ? If the drive is
>> reformatted/converted from SMR to CMR (FORMAT WITH PRESET), the other zone
>> limits may be set already, no ?
>
> blk_validate_zoned_limits already takes care of that.
I do not think it does:
static int blk_validate_zoned_limits(struct queue_limits *lim)
{
if (!lim->zoned) {
if (WARN_ON_ONCE(lim->max_open_zones) ||
WARN_ON_ONCE(lim->max_active_zones) ||
WARN_ON_ONCE(lim->zone_write_granularity) ||
WARN_ON_ONCE(lim->max_zone_append_sectors))
return -EINVAL;
return 0;
}
...
So setting lim->zoned to false without clearing the other limits potentially
will trigger warnings...
--
Damien Le Moal
Western Digital Research
Re: [PATCH 07/26] loop: fold loop_update_rotational into loop_reconfigure_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > This prepares for moving the rotational flag into the queue_limits and > also fixes it for the case where the loop device is backed by a block > device. > > Signed-off-by: Christoph Hellwig Looks good to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 06/26] loop: also use the default block size from an underlying block device
On 6/11/24 2:19 PM, Christoph Hellwig wrote:
> Fix the code in loop_reconfigure_limits to pick a default block size for
> O_DIRECT file descriptors to also work when the loop device sits on top
> of a block device and not just on a regular file on a block device based
> file system.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/block/loop.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 4f6d8514d19bd6..d7cf6bbbfb1b86 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -988,10 +988,16 @@ static int loop_reconfigure_limits(struct loop_device
> *lo, unsigned short bsize)
> {
> struct file *file = lo->lo_backing_file;
> struct inode *inode = file->f_mapping->host;
> + struct block_device *backing_bdev = NULL;
> struct queue_limits lim;
>
> + if (S_ISBLK(inode->i_mode))
> + backing_bdev = I_BDEV(inode);
> + else if (inode->i_sb->s_bdev)
> + backing_bdev = inode->i_sb->s_bdev;
> +
Why not move this hunk inside the below "if" ? (backing_dev declaration can go
there too).
> if (!bsize)
> - bsize = loop_default_blocksize(lo, inode->i_sb->s_bdev);
> + bsize = loop_default_blocksize(lo, backing_bdev);
>
> lim = queue_limits_start_update(lo->lo_queue);
> lim.logical_block_size = bsize;
--
Damien Le Moal
Western Digital Research
Re: [PATCH 05/26] loop: regularize upgrading the lock size for direct I/O
On 6/11/24 2:19 PM, Christoph Hellwig wrote:
> The LOOP_CONFIGURE path automatically upgrades the block size to that
> of the underlying file for O_DIRECT file descriptors, but the
> LOOP_SET_BLOCK_SIZE path does not. Fix this by lifting the code to
> pick the block size into common code.
s/lock/block in the commit title.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/block/loop.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c658282454af1b..4f6d8514d19bd6 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -975,10 +975,24 @@ loop_set_status_from_info(struct loop_device *lo,
> return 0;
> }
>
> +static unsigned short loop_default_blocksize(struct loop_device *lo,
> + struct block_device *backing_bdev)
> +{
> + /* In case of direct I/O, match underlying block size */
> + if ((lo->lo_backing_file->f_flags & O_DIRECT) && backing_bdev)
> + return bdev_logical_block_size(backing_bdev);
> + return 512;
Nit: SECTOR_SIZE ?
> +}
> +
> static int loop_reconfigure_limits(struct loop_device *lo, unsigned short
> bsize)
> {
> + struct file *file = lo->lo_backing_file;
> + struct inode *inode = file->f_mapping->host;
> struct queue_limits lim;
>
> + if (!bsize)
> + bsize = loop_default_blocksize(lo, inode->i_sb->s_bdev);
If bsize is specified and there is a backing dev used with direct IO, should it
be checked that bsize is a multiple of bdev_logical_block_size(backing_bdev) ?
> +
> lim = queue_limits_start_update(lo->lo_queue);
> lim.logical_block_size = bsize;
> lim.physical_block_size = bsize;
> @@ -997,7 +1011,6 @@ static int loop_configure(struct loop_device *lo,
> blk_mode_t mode,
> int error;
> loff_t size;
> bool partscan;
> - unsigned short bsize;
> bool is_loop;
>
> if (!file)
> @@ -1076,15 +1089,7 @@ static int loop_configure(struct loop_device *lo,
> blk_mode_t mode,
> if (!(lo->lo_flags & LO_FLAGS_READ_ONLY) && file->f_op->fsync)
> blk_queue_write_cache(lo->lo_queue, true, false);
>
> - if (config->block_size)
> - bsize = config->block_size;
> - else if ((lo->lo_backing_file->f_flags & O_DIRECT) &&
> inode->i_sb->s_bdev)
> - /* In case of direct I/O, match underlying block size */
> - bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
> - else
> - bsize = 512;
> -
> - error = loop_reconfigure_limits(lo, bsize);
> + error = loop_reconfigure_limits(lo, config->block_size);
> if (WARN_ON_ONCE(error))
> goto out_unlock;
>
--
Damien Le Moal
Western Digital Research
Re: [PATCH 04/26] loop: always update discard settings in loop_reconfigure_limits
On 6/11/24 2:19 PM, Christoph Hellwig wrote: > Simplify loop_reconfigure_limits by always updating the discard limits. > This adds a little more work to loop_set_block_size, but doesn't change > the outcome as the discard flag won't change. > > Signed-off-by: Christoph Hellwig Looks OK to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 03/26] loop: stop using loop_reconfigure_limits in __loop_clr_fd
On 6/11/24 2:19 PM, Christoph Hellwig wrote:
> __loop_clr_fd wants to clear all settings on the device. Prepare for
> moving more settings into the block limits by open coding
> loop_reconfigure_limits.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/block/loop.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 93780f41646b75..93a49c40a31a71 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1133,6 +1133,7 @@ static int loop_configure(struct loop_device *lo,
> blk_mode_t mode,
>
> static void __loop_clr_fd(struct loop_device *lo, bool release)
> {
> + struct queue_limits lim;
> struct file *filp;
> gfp_t gfp = lo->old_gfp_mask;
>
> @@ -1156,7 +1157,14 @@ static void __loop_clr_fd(struct loop_device *lo, bool
> release)
> lo->lo_offset = 0;
> lo->lo_sizelimit = 0;
> memset(lo->lo_file_name, 0, LO_NAME_SIZE);
> - loop_reconfigure_limits(lo, 512, false);
> +
> + /* reset the block size to the default */
> + lim = queue_limits_start_update(lo->lo_queue);
> + lim.logical_block_size = 512;
Nit: SECTOR_SIZE ? maybe ?
> + lim.physical_block_size = 512;
> + lim.io_min = 512;
> + queue_limits_commit_update(lo->lo_queue, );
> +
> invalidate_disk(lo->lo_disk);
> loop_sysfs_exit(lo);
> /* let user-space know about this change */
Otherwise, looks OK to me.
Reviewed-by: Damien Le Moal
--
Damien Le Moal
Western Digital Research
Re: [PATCH 02/26] sd: move zone limits setup out of sd_read_block_characteristics
On 6/11/24 2:19 PM, Christoph Hellwig wrote:
> Move a bit of code that sets up the zone flag and the write granularity
> into sd_zbc_read_zones to be with the rest of the zoned limits.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/sd.c | 21 +
> drivers/scsi/sd_zbc.c | 13 -
> 2 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 85b45345a27739..5bfed61c70db8f 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3308,29 +3308,10 @@ static void sd_read_block_characteristics(struct
> scsi_disk *sdkp,
> blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
> }
>
> -
> -#ifdef CONFIG_BLK_DEV_ZONED /* sd_probe rejects ZBD devices early otherwise
> */
> - if (sdkp->device->type == TYPE_ZBC) {
> - lim->zoned = true;
> -
> - /*
> - * Per ZBC and ZAC specifications, writes in sequential write
> - * required zones of host-managed devices must be aligned to
> - * the device physical block size.
> - */
> - lim->zone_write_granularity = sdkp->physical_block_size;
> - } else {
> - /*
> - * Host-aware devices are treated as conventional.
> - */
> - lim->zoned = false;
> - }
> -#endif /* CONFIG_BLK_DEV_ZONED */
> -
> if (!sdkp->first_scan)
> return;
>
> - if (lim->zoned)
> + if (sdkp->device->type == TYPE_ZBC)
Nit: use sd_is_zoned() here ?
> sd_printk(KERN_NOTICE, sdkp, "Host-managed zoned block
> device\n");
> else if (sdkp->zoned == 1)
> sd_printk(KERN_NOTICE, sdkp, "Host-aware SMR disk used as
> regular disk\n");
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 422eaed8457227..e9501db0450be3 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -598,8 +598,19 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, struct
> queue_limits *lim,
> u32 zone_blocks = 0;
> int ret;
>
> - if (!sd_is_zoned(sdkp))
> + if (!sd_is_zoned(sdkp)) {
> + lim->zoned = false;
Maybe we should clear the other zone related limits here ? If the drive is
reformatted/converted from SMR to CMR (FORMAT WITH PRESET), the other zone
limits may be set already, no ?
> return 0;
> + }
> +
> + lim->zoned = true;
> +
> + /*
> + * Per ZBC and ZAC specifications, writes in sequential write required
> + * zones of host-managed devices must be aligned to the device physical
> + * block size.
> + */
> + lim->zone_write_granularity = sdkp->physical_block_size;
>
> /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */
> sdkp->device->use_16_for_rw = 1;
--
Damien Le Moal
Western Digital Research
Re: [PATCH 01/26] sd: fix sd_is_zoned
On 6/11/24 2:19 PM, Christoph Hellwig wrote:
> Since commit 7437bb73f087 ("block: remove support for the host aware zone
> model"), only ZBC devices expose a zoned access model. sd_is_zoned is
> used to check for that and thus return false for host aware devices.
>
> Fixes: 7437bb73f087 ("block: remove support for the host aware zone model")
> Signed-off-by: Christoph Hellwig
Looks good.
Reviewed-by: Damien Le Moal
--
Damien Le Moal
Western Digital Research
Re: [PATCH] ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K
On 6/6/24 20:14, Michael Ellerman wrote:
> The pata_macio driver advertises a max_segment_size of 0xff00, because
> the hardware doesn't cope with requests >= 64K.
>
> However the SCSI core requires max_segment_size to be at least
> PAGE_SIZE, which is a problem for pata_macio when the kernel is built
> with 64K pages.
>
> In older kernels the SCSI core would just increase the segment size to
> be equal to PAGE_SIZE, however since the commit tagged below it causes a
> warning and the device fails to probe:
>
> WARNING: CPU: 0 PID: 26 at block/blk-settings.c:202
> .blk_validate_limits+0x2f8/0x35c
> CPU: 0 PID: 26 Comm: kworker/u4:1 Not tainted 6.10.0-rc1 #1
> Hardware name: PowerMac7,2 PPC970 0x390202 PowerMac
> ...
> NIP .blk_validate_limits+0x2f8/0x35c
> LR .blk_alloc_queue+0xc0/0x2f8
> Call Trace:
> .blk_alloc_queue+0xc0/0x2f8
> .blk_mq_alloc_queue+0x60/0xf8
> .scsi_alloc_sdev+0x208/0x3c0
> .scsi_probe_and_add_lun+0x314/0x52c
> .__scsi_add_device+0x170/0x1a4
> .ata_scsi_scan_host+0x2bc/0x3e4
> .async_port_probe+0x6c/0xa0
> .async_run_entry_fn+0x60/0x1bc
> .process_one_work+0x228/0x510
> .worker_thread+0x360/0x530
> .kthread+0x134/0x13c
> .start_kernel_thread+0x10/0x14
> ...
> scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices
> might not be configured
>
> Although the hardware can't cope with a 64K segment, the driver
> already deals with that internally by splitting large requests in
> pata_macio_qc_prep(). That is how the driver has managed to function
> until now on 64K kernels.
>
> So fix the driver to advertise a max_segment_size of 64K, which avoids
> the warning and keeps the SCSI core happy.
>
> Fixes: afd53a3d8528 ("scsi: core: Initialize scsi midlayer limits before
> allocating the queue")
> Reported-by: Guenter Roeck
> Closes:
> https://lore.kernel.org/all/ce2bf6af-4382-4fe1-b392-cc6829f5c...@roeck-us.net/
> Reported-by: Doru Iorgulescu
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218858
> Signed-off-by: Michael Ellerman
> Reviewed-by: Christoph Hellwig
Looks good.
Reviewed-by: Damien Le Moal
> ---
> drivers/ata/pata_macio.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
> index 817838e2f70e..3cb455a32d92 100644
> --- a/drivers/ata/pata_macio.c
> +++ b/drivers/ata/pata_macio.c
> @@ -915,10 +915,13 @@ static const struct scsi_host_template pata_macio_sht =
> {
> .sg_tablesize = MAX_DCMDS,
> /* We may not need that strict one */
> .dma_boundary = ATA_DMA_BOUNDARY,
> - /* Not sure what the real max is but we know it's less than 64K, let's
> - * use 64K minus 256
> + /*
> + * The SCSI core requires the segment size to cover at least a page, so
> + * for 64K page size kernels this must be at least 64K. However the
> + * hardware can't handle 64K, so pata_macio_qc_prep() will split large
> + * requests.
>*/
> - .max_segment_size = MAX_DBDMA_SEG,
> + .max_segment_size = SZ_64K,
> .device_configure = pata_macio_device_configure,
> .sdev_groups= ata_common_sdev_groups,
> .can_queue = ATA_DEF_QUEUE,
--
Damien Le Moal
Western Digital Research
Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
On 6/3/24 00:57, Andy Shevchenko wrote:
> Make two APIs look similar. Hence convert match_string() to be
> a 2-argument macro. In order to avoid unneeded churn, convert
> all users as well. There is no functional change intended.
>
> Signed-off-by: Andy Shevchenko
> ---
[...]
> diff --git a/drivers/ata/pata_hpt366.c b/drivers/ata/pata_hpt366.c
> index bdccd1ba1524..8134f9290791 100644
> --- a/drivers/ata/pata_hpt366.c
> +++ b/drivers/ata/pata_hpt366.c
> @@ -178,7 +178,7 @@ static int hpt_dma_blacklisted(const struct ata_device
> *dev, char *modestr,
>
> ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
>
> - i = match_string(list, -1, model_num);
> + i = __match_string(list, -1, model_num);
> if (i >= 0) {
> ata_dev_warn(dev, "%s is not supported for %s\n", modestr,
> list[i]);
> return 1;
> diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
> index c0329cf01135..2d0b659bbd65 100644
> --- a/drivers/ata/pata_hpt37x.c
> +++ b/drivers/ata/pata_hpt37x.c
> @@ -226,7 +226,7 @@ static int hpt_dma_blacklisted(const struct ata_device
> *dev, char *modestr,
>
> ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
>
> - i = match_string(list, -1, model_num);
> + i = __match_string(list, -1, model_num);
> if (i >= 0) {
> ata_dev_warn(dev, "%s is not supported for %s\n",
>modestr, list[i]);
Looks good to me.
Acked-by: Damien Le Moal# drivers/ata/
--
Damien Le Moal
Western Digital Research
Re: [PATCH treewide 0/9] Remove obsolete IDE headers
On 8/18/23 01:07, Geert Uytterhoeven wrote: > Hi all, > > This patch series removes all unused headers and > . was still included by 3 PATA > drivers for m68k platforms, but without any real need. > > The first 5 patches have no dependencies. > The last patch depends on the 3 pata patches. > > Thanks for your comments! > > Geert Uytterhoeven (9): > ARM: Remove > parisc: Remove > powerpc: Remove > sparc: Remove > asm-generic: Remove ide_iops.h > ata: pata_buddha: Remove #include > ata: pata_falcon: Remove #include > ata: pata_gayle: Remove #include > m68k: Remove Applied to libata for-6.6. Thanks ! -- Damien Le Moal Western Digital Research
Re: [PATCH 1/9] ARM: Remove
On 8/18/23 01:07, Geert Uytterhoeven wrote:
> As of commit b7fb14d3ac63117e ("ide: remove the legacy ide driver") in
> v5.14, there are no more generic users of .
>
> Signed-off-by: Geert Uytterhoeven
Looks good to me. All patches are reviewed or acked, except this one.
Can I get an ack from arm folks ?
> ---
> arch/arm/include/asm/ide.h | 24
> 1 file changed, 24 deletions(-)
> delete mode 100644 arch/arm/include/asm/ide.h
>
> diff --git a/arch/arm/include/asm/ide.h b/arch/arm/include/asm/ide.h
> deleted file mode 100644
> index a81e0b0d6747aa2f..
> --- a/arch/arm/include/asm/ide.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * arch/arm/include/asm/ide.h
> - *
> - * Copyright (C) 1994-1996 Linus Torvalds & authors
> - */
> -
> -/*
> - * This file contains the ARM architecture specific IDE code.
> - */
> -
> -#ifndef __ASMARM_IDE_H
> -#define __ASMARM_IDE_H
> -
> -#ifdef __KERNEL__
> -
> -#define __ide_mm_insw(port,addr,len) readsw(port,addr,len)
> -#define __ide_mm_insl(port,addr,len) readsl(port,addr,len)
> -#define __ide_mm_outsw(port,addr,len)writesw(port,addr,len)
> -#define __ide_mm_outsl(port,addr,len)writesl(port,addr,len)
> -
> -#endif /* __KERNEL__ */
> -
> -#endif /* __ASMARM_IDE_H */
--
Damien Le Moal
Western Digital Research
Re: [PATCH v2 92/92] fs: rename i_ctime field to __i_ctime
On 7/6/23 03:58, Jeff Layton wrote: > Now that everything in-tree is converted to use the accessor functions, > rename the i_ctime field in the inode to discourage direct access. > > Signed-off-by: Jeff Layton Looks OK to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH v2 08/92] fs: new helper: simple_rename_timestamp
On 7/6/23 03:58, Jeff Layton wrote:
> A rename potentially involves updating 4 different inode timestamps. Add
> a function that handles the details sanely, and convert the libfs.c
> callers to use it.
>
> Signed-off-by: Jeff Layton
> ---
> fs/libfs.c | 36 +++-
> include/linux/fs.h | 2 ++
> 2 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index a7e56baf8bbd..9ee79668c909 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -692,6 +692,31 @@ int simple_rmdir(struct inode *dir, struct dentry
> *dentry)
> }
> EXPORT_SYMBOL(simple_rmdir);
>
> +/**
> + * simple_rename_timestamp - update the various inode timestamps for rename
> + * @old_dir: old parent directory
> + * @old_dentry: dentry that is being renamed
> + * @new_dir: new parent directory
> + * @new_dentry: target for rename
> + *
> + * POSIX mandates that the old and new parent directories have their ctime
> and
> + * mtime updated, and that inodes of @old_dentry and @new_dentry (if any),
> have
> + * their ctime updated.
> + */
> +void simple_rename_timestamp(struct inode *old_dir, struct dentry
> *old_dentry,
> + struct inode *new_dir, struct dentry *new_dentry)
> +{
> + struct inode *newino = d_inode(new_dentry);
> +
> + old_dir->i_mtime = inode_set_ctime_current(old_dir);
> + if (new_dir != old_dir)
> + new_dir->i_mtime = inode_set_ctime_current(new_dir);
> + inode_set_ctime_current(d_inode(old_dentry));
> + if (newino)
> + inode_set_ctime_current(newino);
> +}
> +EXPORT_SYMBOL_GPL(simple_rename_timestamp);
> +
> int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
> struct inode *new_dir, struct dentry *new_dentry)
> {
> @@ -707,11 +732,7 @@ int simple_rename_exchange(struct inode *old_dir, struct
> dentry *old_dentry,
> inc_nlink(old_dir);
> }
> }
> - old_dir->i_ctime = old_dir->i_mtime =
> - new_dir->i_ctime = new_dir->i_mtime =
> - d_inode(old_dentry)->i_ctime =
> - d_inode(new_dentry)->i_ctime = current_time(old_dir);
> -
> + simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
This is somewhat changing the current behavior: before the patch, the mtime and
ctime of old_dir, new_dir and the inodes associated with the dentries are always
equal. But given that simple_rename_timestamp() calls inode_set_ctime_current()
4 times, the times could potentially be different.
I am not sure if that is an issue, but it seems that calling
inode_set_ctime_current() once, recording the "now" time it sets and using that
value to set all times may be more efficient and preserve the existing behavior.
> return 0;
> }
> EXPORT_SYMBOL_GPL(simple_rename_exchange);
> @@ -720,7 +741,6 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
> *old_dir,
> struct dentry *old_dentry, struct inode *new_dir,
> struct dentry *new_dentry, unsigned int flags)
> {
> - struct inode *inode = d_inode(old_dentry);
> int they_are_dirs = d_is_dir(old_dentry);
>
> if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
> @@ -743,9 +763,7 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
> *old_dir,
> inc_nlink(new_dir);
> }
>
> - old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
> - new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
> -
> + simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
> return 0;
> }
> EXPORT_SYMBOL(simple_rename);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bdfbd11a5811..14e38bd900f1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2979,6 +2979,8 @@ extern int simple_open(struct inode *inode, struct file
> *file);
> extern int simple_link(struct dentry *, struct inode *, struct dentry *);
> extern int simple_unlink(struct inode *, struct dentry *);
> extern int simple_rmdir(struct inode *, struct dentry *);
> +void simple_rename_timestamp(struct inode *old_dir, struct dentry
> *old_dentry,
> + struct inode *new_dir, struct dentry *new_dentry);
> extern int simple_rename_exchange(struct inode *old_dir, struct dentry
> *old_dentry,
> struct inode *new_dir, struct dentry
> *new_dentry);
> extern int simple_rename(struct mnt_idmap *, struct inode *,
--
Damien Le Moal
Western Digital Research
Re: [PATCH v2 07/92] fs: add ctime accessors infrastructure
On 7/6/23 03:58, Jeff Layton wrote: > struct timespec64 has unused bits in the tv_nsec field that can be used > for other purposes. In future patches, we're going to change how the > inode->i_ctime is accessed in certain inodes in order to make use of > them. In order to do that safely though, we'll need to eradicate raw > accesses of the inode->i_ctime field from the kernel. > > Add new accessor functions for the ctime that we use to replace them. > > Reviewed-by: Jan Kara > Reviewed-by: Luis Chamberlain > Signed-off-by: Jeff Layton Looks OK to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH 01/79] fs: add ctime accessors infrastructure
On 6/21/23 23:45, Jeff Layton wrote: > struct timespec64 has unused bits in the tv_nsec field that can be used > for other purposes. In future patches, we're going to change how the > inode->i_ctime is accessed in certain inodes in order to make use of > them. In order to do that safely though, we'll need to eradicate raw > accesses of the inode->i_ctime field from the kernel. > > Add new accessor functions for the ctime that we can use to replace them. > > Signed-off-by: Jeff Layton [...] > +/** > + * inode_ctime_peek - fetch the current ctime from the inode > + * @inode: inode from which to fetch ctime > + * > + * Grab the current ctime from the inode and return it. > + */ > +static inline struct timespec64 inode_ctime_peek(const struct inode *inode) To be consistent with inode_ctime_set(), why not call this one inode_ctime_get() ? Also, inode_set_ctime() & inode_get_ctime() may be a little more natural. But no strong opinion about that though. -- Damien Le Moal Western Digital Research
Re: [PATCH v3] powerpc: macio: Make remove callback of macio driver void returned
On 2/1/23 23:36, Dawei Li wrote:
> Commit fc7a6209d571 ("bus: Make remove callback return void") forces
> bus_type::remove be void-returned, it doesn't make much sense for any
> bus based driver implementing remove callbalk to return non-void to
> its caller.
>
> This change is for macio bus based drivers.
>
> Signed-off-by: Dawei Li
> ---
> v2 -> v3
> - Rebased on latest powerpc/next.
> - cc' to relevant subsysem lists.
>
> v1 -> v2
> - Revert unneeded changes.
> - Rebased on latest powerpc/next.
>
> v1
> -
> https://lore.kernel.org/all/tycp286mb2323fcdc7ecd87f8d97cb74bca...@tycp286mb2323.jpnp286.prod.outlook.com/
> ---
> arch/powerpc/include/asm/macio.h| 2 +-
> drivers/ata/pata_macio.c| 4 +---
> drivers/macintosh/rack-meter.c | 4 +---
> drivers/net/ethernet/apple/bmac.c | 4 +---
> drivers/net/ethernet/apple/mace.c | 4 +---
> drivers/net/wireless/intersil/orinoco/airport.c | 4 +---
> drivers/scsi/mac53c94.c | 5 +
> drivers/scsi/mesh.c | 5 +
> drivers/tty/serial/pmac_zilog.c | 7 ++-
> sound/aoa/soundbus/i2sbus/core.c | 4 +---
> 10 files changed, 11 insertions(+), 32 deletions(-)
For the ata bits:
Acked-by: Damien Le Moal
--
Damien Le Moal
Western Digital Research
Re: [PATCH v2] powerpc: select HAVE_PATA_PLATFORM in PPC instead of creating a PPC dependency
On 2022/09/11 21:41, Arnd Bergmann wrote: > On Sun, Sep 11, 2022, at 1:54 PM, Damien Le Moal wrote: >> On 2022/09/09 20:31, Arnd Bergmann wrote: >>> >>> config PATA_PLATFORM >>> - tristate "Generic platform device PATA support" >>> - depends on EXPERT || PPC || HAVE_PATA_PLATFORM >>> + tristate "Generic platform device PATA support" if EXPERT || >>> HAVE_PATA_PLATFORM >> >> Shouldn't this be: >> >> tristate "Generic platform device PATA support" if EXPERT || PPC >> >> ? >> >> And while at it, it would be nice to add "|| COMPILE_TEST" too. > > The idea was that this can be selected by CONFIG_PATA_OF_PLATFORM > in any configuration that has CONFIG_OF enabled. Since PPC > has CONFIG_OF enabled unconditionally, there is no need to > make this option visible separately. > > Same for compile-testing: since CONFIG_OF can be enabled on > any architecture, PATA_OF_PLATFORM is already covered by > allmodconfig builds anywhere. The separate HAVE_PATA_PLATFORM > is only needed for machines that want the non-OF pata-platform > module (sh, m68k-mac, mips-sibyte arm-s3c-simtec). Got it. Thanks for the details. > >Arnd -- Damien Le Moal Western Digital Research
Re: [PATCH v2] powerpc: select HAVE_PATA_PLATFORM in PPC instead of creating a PPC dependency
On 2022/09/09 20:31, Arnd Bergmann wrote:
> On Fri, Sep 9, 2022, at 1:19 PM, Christophe Leroy wrote:
>> Le 09/09/2022 à 13:09, Arnd Bergmann a écrit :
>>> On Fri, Sep 9, 2022, at 11:03 AM, Lukas Bulwahn wrote:
>>>
>>> I don't see a single powerpc machine that creates a
>>> name="pata_platform" platform_device. I suspect this was
>>> only needed bwfore 2007 commit 9cd55be4d223 ("[POWERPC] pasemi:
>>> Move electra-ide to pata_of_platform"), so the "|| PPC"
>>> bit should just get removed without adding the HAVE_PATA_PLATFORM
>>> bit.
>>
>> But that was added in 2008 by commit 61f7162117d4 ("libata:
>> pata_of_platform: OF-Platform PATA device driver")
>
> Ah, I see. In that case, I think we should probably just always
> allow PATA_OF_PLATFORM to be enabled regardless of
> HAVE_PATA_PLATFORM, something like
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 1c9f4fb2595d..c93d97455744 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -1102,8 +1102,7 @@ config PATA_PCMCIA
> If unsure, say N.
>
> config PATA_PLATFORM
> - tristate "Generic platform device PATA support"
> - depends on EXPERT || PPC || HAVE_PATA_PLATFORM
> + tristate "Generic platform device PATA support" if EXPERT ||
> HAVE_PATA_PLATFORM
Shouldn't this be:
tristate "Generic platform device PATA support" if EXPERT || PPC
?
And while at it, it would be nice to add "|| COMPILE_TEST" too.
> help
> This option enables support for generic directly connected ATA
> devices commonly found on embedded systems.
> @@ -1112,7 +,8 @@ config PATA_PLATFORM
>
> config PATA_OF_PLATFORM
> tristate "OpenFirmware platform device PATA support"
> - depends on PATA_PLATFORM && OF
> + depends on OF
> + select PATA_PLATFORM
> help
> This option enables support for generic directly connected ATA
> devices commonly found on embedded systems with OpenFirmware
>
> and then also drop the "select HAVE_PATA_PLATFORM" from
> arm64 and arm/versatile.
>
> Or we can go one step further, and either split out the
> 'pata_platform_driver' into a separate file from
> '__pata_platform_probe', or merge pata_of_platform.c
> back into pata_platform.c.
>
> Arnd
--
Damien Le Moal
Western Digital Research
Re: [PATCH v1 2/4] powerpc/mpc5xxx: Switch mpc5xxx_get_bus_frequency() to use fwnode
On 2022/05/04 22:44, Andy Shevchenko wrote: > Switch mpc5xxx_get_bus_frequency() to use fwnode in order to help > cleaning up other parts of the kernel from OF specific code. > > No functional change intended. > > Signed-off-by: Andy Shevchenko For the pata bits, Acked-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [PATCH] scsi: ibmvfc: replace snprintf with sysfs_emit
On 2/9/22 09:43, davidcomponent...@gmail.com wrote:
> From: Yang Guang
>
> coccinelle report:
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3453:8-16:
> WARNING: use scnprintf or sprintf
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3416:8-16:
> WARNING: use scnprintf or sprintf
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3436:8-16:
> WARNING: use scnprintf or sprintf
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3426:8-16:
> WARNING: use scnprintf or sprintf
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3445:8-16:
> WARNING: use scnprintf or sprintf
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3406:8-16:
> WARNING: use scnprintf or sprintf
>
> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
>
> Reported-by: Zeal Robot
> Signed-off-by: Yang Guang
> Signed-off-by: David Yang
> ---
> drivers/scsi/ibmvscsi/ibmvfc.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index d0eab5700dc5..d5a197d17e0a 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -3403,7 +3403,7 @@ static ssize_t ibmvfc_show_host_partition_name(struct
> device *dev,
> struct Scsi_Host *shost = class_to_shost(dev);
> struct ibmvfc_host *vhost = shost_priv(shost);
>
> - return snprintf(buf, PAGE_SIZE, "%s\n",
> + return sysfs_emit(buf, "%s\n",
> vhost->login_buf->resp.partition_name);
> }
>
> @@ -3413,7 +3413,7 @@ static ssize_t ibmvfc_show_host_device_name(struct
> device *dev,
> struct Scsi_Host *shost = class_to_shost(dev);
> struct ibmvfc_host *vhost = shost_priv(shost);
>
> - return snprintf(buf, PAGE_SIZE, "%s\n",
> + return sysfs_emit(buf, "%s\n",
> vhost->login_buf->resp.device_name);
> }
>
> @@ -3423,7 +3423,7 @@ static ssize_t ibmvfc_show_host_loc_code(struct device
> *dev,
> struct Scsi_Host *shost = class_to_shost(dev);
> struct ibmvfc_host *vhost = shost_priv(shost);
>
> - return snprintf(buf, PAGE_SIZE, "%s\n",
> + return sysfs_emit(buf, "%s\n",
> vhost->login_buf->resp.port_loc_code);
> }
>
> @@ -3433,7 +3433,7 @@ static ssize_t ibmvfc_show_host_drc_name(struct device
> *dev,
> struct Scsi_Host *shost = class_to_shost(dev);
> struct ibmvfc_host *vhost = shost_priv(shost);
>
> - return snprintf(buf, PAGE_SIZE, "%s\n",
> + return sysfs_emit(buf, "%s\n",
> vhost->login_buf->resp.drc_name);
> }
>
> @@ -3442,7 +3442,7 @@ static ssize_t ibmvfc_show_host_npiv_version(struct
> device *dev,
> {
> struct Scsi_Host *shost = class_to_shost(dev);
> struct ibmvfc_host *vhost = shost_priv(shost);
> - return snprintf(buf, PAGE_SIZE, "%d\n",
> be32_to_cpu(vhost->login_buf->resp.version));
> + return sysfs_emit(buf, "%d\n",
> be32_to_cpu(vhost->login_buf->resp.version));
The format should be %u, not %d. And while at it, please add a blank
line after the declarations.
> }
>
> static ssize_t ibmvfc_show_host_capabilities(struct device *dev,
> @@ -3450,7 +3450,7 @@ static ssize_t ibmvfc_show_host_capabilities(struct
> device *dev,
> {
> struct Scsi_Host *shost = class_to_shost(dev);
> struct ibmvfc_host *vhost = shost_priv(shost);
> - return snprintf(buf, PAGE_SIZE, "%llx\n",
> be64_to_cpu(vhost->login_buf->resp.capabilities));
> + return sysfs_emit(buf, "%llx\n",
> be64_to_cpu(vhost->login_buf->resp.capabilities));
> }
Ditto for the blank line.
>
> /**
> @@ -3471,7 +3471,7 @@ static ssize_t ibmvfc_show_log_level(struct device *dev,
> int len;
>
> spin_lock_irqsave(shost->host_lock, flags);
> - len = snprintf(buf, PAGE_SIZE, "%d\n", vhost->log_level);
> + len = sysfs_emit(buf, "%d\n", vhost->log_level);
> spin_unlock_irqrestore(shost->host_lock, flags);
> return len;
> }
> @@ -3509,7 +3509,7 @@ static ssize_t ibmvfc_show_scsi_channels(struct device
> *dev,
> int len;
>
> spin_lock_irqsave(shost->host_lock, flags);
> - len = snprintf(buf, PAGE_SIZE, "%d\n", vhost->client_scsi_channels);
> + len = sysfs_emit(buf, "%d\n", vhost->client_scsi_channels);
> spin_unlock_irqrestore(shost->host_lock, flags);
> return len;
> }
--
Damien Le Moal
Western Digital Research
Re: [PATCH] ata: sata_fsl: fix sscanf() and sysfs_emit() format strings
On 2/8/22 15:46, Damien Le Moal wrote:
> Use the %u format for unsigned int parameters handling with sscanf() and
> sysfs_emit() to avoid compilation warnings. In
> fsl_sata_rx_watermark_store(), the call to sscanf() to parse a single
> argument is replaced with a call to kstrtouint().
>
> While at it, also replace the printk(KERN_ERR) calls with dev_err()
> calls and fix blank lines in fsl_sata_rx_watermark_store().
>
> Reported-by: kernel test robot
> Signed-off-by: Damien Le Moal
> ---
> drivers/ata/sata_fsl.c | 16 +++-
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index da0152116d9f..556034a15430 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -322,7 +322,7 @@ static void fsl_sata_set_irq_coalescing(struct ata_host
> *host,
> static ssize_t fsl_sata_intr_coalescing_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - return sysfs_emit(buf, "%d %d\n",
> + return sysfs_emit(buf, "%u %u\n",
> intr_coalescing_count, intr_coalescing_ticks);
> }
>
> @@ -332,10 +332,8 @@ static ssize_t fsl_sata_intr_coalescing_store(struct
> device *dev,
> {
> unsigned int coalescing_count, coalescing_ticks;
>
> - if (sscanf(buf, "%d%d",
> - _count,
> - _ticks) != 2) {
> - printk(KERN_ERR "fsl-sata: wrong parameter format.\n");
> + if (sscanf(buf, "%u%u", _count, _ticks) != 2) {
PPC folks,
The "%u%u" sscanf() format above seems totally bogus to me. How could 2
unsigned int without a separating characters be parsed as such ? Surely,
a separation character is needed, no ?
I cannot find any documentation on what the intr_coalescing sysfs
attribute format should be... Please have a look.
> + dev_err(dev, "fsl-sata: wrong parameter format.\n");
> return -EINVAL;
> }
>
> @@ -359,7 +357,7 @@ static ssize_t fsl_sata_rx_watermark_show(struct device
> *dev,
> rx_watermark &= 0x1f;
> spin_unlock_irqrestore(>lock, flags);
>
> - return sysfs_emit(buf, "%d\n", rx_watermark);
> + return sysfs_emit(buf, "%u\n", rx_watermark);
> }
>
> static ssize_t fsl_sata_rx_watermark_store(struct device *dev,
> @@ -373,8 +371,8 @@ static ssize_t fsl_sata_rx_watermark_store(struct device
> *dev,
> void __iomem *csr_base = host_priv->csr_base;
> u32 temp;
>
> - if (sscanf(buf, "%d", _watermark) != 1) {
> - printk(KERN_ERR "fsl-sata: wrong parameter format.\n");
> + if (kstrtouint(buf, 10, _watermark) < 0) {
> + dev_err(dev, "fsl-sata: wrong parameter format.\n");
> return -EINVAL;
> }
>
> @@ -382,8 +380,8 @@ static ssize_t fsl_sata_rx_watermark_store(struct device
> *dev,
> temp = ioread32(csr_base + TRANSCFG);
> temp &= 0xffe0;
> iowrite32(temp | rx_watermark, csr_base + TRANSCFG);
> -
> spin_unlock_irqrestore(>lock, flags);
> +
> return strlen(buf);
> }
>
--
Damien Le Moal
Western Digital Research
[PATCH] ata: sata_fsl: fix sscanf() and sysfs_emit() format strings
Use the %u format for unsigned int parameters handling with sscanf() and
sysfs_emit() to avoid compilation warnings. In
fsl_sata_rx_watermark_store(), the call to sscanf() to parse a single
argument is replaced with a call to kstrtouint().
While at it, also replace the printk(KERN_ERR) calls with dev_err()
calls and fix blank lines in fsl_sata_rx_watermark_store().
Reported-by: kernel test robot
Signed-off-by: Damien Le Moal
---
drivers/ata/sata_fsl.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index da0152116d9f..556034a15430 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -322,7 +322,7 @@ static void fsl_sata_set_irq_coalescing(struct ata_host
*host,
static ssize_t fsl_sata_intr_coalescing_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return sysfs_emit(buf, "%d %d\n",
+ return sysfs_emit(buf, "%u %u\n",
intr_coalescing_count, intr_coalescing_ticks);
}
@@ -332,10 +332,8 @@ static ssize_t fsl_sata_intr_coalescing_store(struct
device *dev,
{
unsigned int coalescing_count, coalescing_ticks;
- if (sscanf(buf, "%d%d",
- _count,
- _ticks) != 2) {
- printk(KERN_ERR "fsl-sata: wrong parameter format.\n");
+ if (sscanf(buf, "%u%u", _count, _ticks) != 2) {
+ dev_err(dev, "fsl-sata: wrong parameter format.\n");
return -EINVAL;
}
@@ -359,7 +357,7 @@ static ssize_t fsl_sata_rx_watermark_show(struct device
*dev,
rx_watermark &= 0x1f;
spin_unlock_irqrestore(>lock, flags);
- return sysfs_emit(buf, "%d\n", rx_watermark);
+ return sysfs_emit(buf, "%u\n", rx_watermark);
}
static ssize_t fsl_sata_rx_watermark_store(struct device *dev,
@@ -373,8 +371,8 @@ static ssize_t fsl_sata_rx_watermark_store(struct device
*dev,
void __iomem *csr_base = host_priv->csr_base;
u32 temp;
- if (sscanf(buf, "%d", _watermark) != 1) {
- printk(KERN_ERR "fsl-sata: wrong parameter format.\n");
+ if (kstrtouint(buf, 10, _watermark) < 0) {
+ dev_err(dev, "fsl-sata: wrong parameter format.\n");
return -EINVAL;
}
@@ -382,8 +380,8 @@ static ssize_t fsl_sata_rx_watermark_store(struct device
*dev,
temp = ioread32(csr_base + TRANSCFG);
temp &= 0xffe0;
iowrite32(temp | rx_watermark, csr_base + TRANSCFG);
-
spin_unlock_irqrestore(>lock, flags);
+
return strlen(buf);
}
--
2.34.1
Re: [PASEMI] Nemo board doesn't recognize any ATA disks with the pci-v5.16 updates
On 2021/11/10 7:40, Krzysztof Wilczyński wrote:
> [+CC Adding Jens and Damien to get their opinion about the problem at hand]
>
> Hello Jens and Damien,
>
> Sorry to bother both of you, but we are having a problem that most
> definitely requires someone with an extensive expertise in storage,
> as per the quoted message from Christian below:
>
>>>> The Nemo board [1] doesn't recognize any ATA disks with the pci-v5.16
>>>> updates [2].
>>>>
>>>> Error messages:
>>>>
>>>> ata4.00: gc timeout cmd 0xec
>>>> ata4.00: failed to IDENTIFY (I/O error, error_mask=0x4)
>>>> ata1.00: gc timeout cmd 0xec
>>>> ata1.00: failed to IDENTIFY (I/O error, error_mask=0x4)
>>>> ata3.00: gc timeout cmd 0xec
>>>> ata3.00: failed to IDENTIFY (I/O error, error_mask=0x4)
IDENTIFY is the first command sent to a device when it is being probed. This
means that at least the AHCI (is it AHCI ?) adapter found the ports and drives
connected. But the qc timeout indicates that there is no response from the
drive. This could be due to interrupts not being received for the command
completion. One thing to try would be to increase the identify command timeout
to see things simply got slow (for whatever reason) or if indeed there is no
response at all. Note that after the first timeout, normally the port is reset
and the command retried. That does not seem to be the case here. Weird...
Maybe try something like this:
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 1d4a6f1e88cd..16e105bcb899 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -79,7 +79,7 @@ enum {
* take an exceptionally long time to recover from reset.
*/
static const unsigned long ata_eh_reset_timeouts[] = {
- 1, /* most drives spin up by 10sec */
+ 3, /* most drives spin up by 10sec */
1, /* > 99% working drives spin up before 20sec */
35000, /* give > 30 secs of idleness for outlier devices */
5000, /* and sweet one last chance */
Also note that I posted a patch a couple of days ago fixing a qc timeout for
read log commands during device probe. This is not what you are hitting here
though. I have not yet sent this to Linus.
https://lore.kernel.org/linux-ide/20211105073106.422623-1-damien.lem...@opensource.wdc.com/
>
> The error message is also not very detailed and we aren't really sure what
> the issue coming from the PCI sub-system might be causing or leading to
> this.
>
>>>>
>>>> I was able to revert the new pci-v5.16 updates [2]. After a new compiling,
>>>> the kernel recognize all ATA disks correctly.
>>>>
>>>> Could you please check the pci-v5.16 updates [2]?
>>>>
>>>> Please find attached the kernel config.
>>>>
>>>> Thanks,
>>>> Christian
>>>>
>>>> [1] https://en.wikipedia.org/wiki/AmigaOne_X1000
>>>> [2]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
>>
>> Sorry for the breakage, and thank you very much for the report. Can
>> you please collect the complete dmesg logs before and after the
>> pci-v5.16 changes and the "sudo lspci -vv" output from before the
>> changes?
>>
>> You can attach them at https://bugzilla.kernel.org if you don't have
>> a better place to put them.
>>
>> You could attach the kernel config there, too, since it didn't make it
>> to the mailing list (vger may discard them -- see
>> http://vger.kernel.org/majordomo-info.html).
>
> Bjorn and I looked at which commits that went with a recent Pull Request
> from us might be causing this, but we are a little bit at loss, and were
> hoping that you could give us a hand in troubleshooting this.
>
> Thank you in advance!
>
> Krzysztof
>
>
--
Damien Le Moal
Western Digital Research
Re: [PATCH RFC PKS/PMEM 26/58] fs/zonefs: Utilize new kmap_thread()
On 2020/10/10 4:52, ira.we...@intel.com wrote: > From: Ira Weiny > > The kmap() calls in this FS are localized to a single thread. To avoid > the over head of global PKRS updates use the new kmap_thread() call. > > Cc: Damien Le Moal > Cc: Naohiro Aota > Signed-off-by: Ira Weiny > --- > fs/zonefs/super.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c > index 8ec7c8f109d7..2fd6c86beee1 100644 > --- a/fs/zonefs/super.c > +++ b/fs/zonefs/super.c > @@ -1297,7 +1297,7 @@ static int zonefs_read_super(struct super_block *sb) > if (ret) > goto free_page; > > - super = kmap(page); > + super = kmap_thread(page); > > ret = -EINVAL; > if (le32_to_cpu(super->s_magic) != ZONEFS_MAGIC) > @@ -1349,7 +1349,7 @@ static int zonefs_read_super(struct super_block *sb) > ret = 0; > > unmap: > - kunmap(page); > + kunmap_thread(page); > free_page: > __free_page(page); > > acked-by: Damien Le Moal -- Damien Le Moal Western Digital Research
