Re: [PATCH v2 17/21] block, blksnap: snapshot image block device

Re: [PATCH v2 17/21] block, blksnap: snapshot image block device

[Hillf Danton]

On 9 Dec 2022 15:23:27 +0100 Sergei Shtepa <sergei.shtepa@veeam.com>
> Provides the operation of block devices of snapshot images. Read and
> write operations are redirected to the regions of difference blocks for
> block device (struct diff_area).
> 
> Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
> ---

Thanks for your patchset.

> +static int snapimage_kthread_worker_fn(void *param)
> +{
> +	struct snapimage *snapimage = param;
> +	struct bio *bio;
> +	int ret = 0;
> +
> +	while (!kthread_should_stop()) {
> +		bio = get_bio_from_queue(snapimage);
> +		if (!bio) {
> +			schedule_timeout_interruptible(HZ / 100);
> +			continue;
> +		}

Given the wake_up_process() below, s$HZ / 100$HZ * 1000$ to avoid
unnecessary wakeups [1].

And because of no signal handling added, use schedule_timeout_idle() instead.

[1] https://lore.kernel.org/lkml/20210419085027.761150-2-elver@google.com/

> +
> +		snapimage_process_bio(snapimage, bio);
> +	}
> +
> +	while ((bio = get_bio_from_queue(snapimage)))
> +		snapimage_process_bio(snapimage, bio);
> +
> +	return ret;
> +}
> +
> +static void snapimage_submit_bio(struct bio *bio)
> +{
> +	struct snapimage *snapimage = bio->bi_bdev->bd_disk->private_data;
> +	gfp_t gfp = GFP_NOIO;
> +
> +	if (bio->bi_opf & REQ_NOWAIT)
> +		gfp |= GFP_NOWAIT;
> +	if (snapimage->is_ready) {
> +		spin_lock(&snapimage->queue_lock);
> +		bio_list_add(&snapimage->queue, bio);
> +		spin_unlock(&snapimage->queue_lock);
> +
> +		wake_up_process(snapimage->worker);
> +	} else
> +		bio_io_error(bio);
> +}

Re: [PATCH v2 18/21] block, blksnap: snapshot

[Hillf Danton]

On 9 Dec 2022 15:23:28 +0100 Sergei Shtepa <sergei.shtepa@veeam.com>
> +int snapshot_create(struct blk_snap_dev *dev_id_array, unsigned int count,
> +		    uuid_t *id)
> +{
> +	struct snapshot *snapshot = NULL;
> +	int ret;
> +	unsigned int inx;
> +
> +	pr_info("Create snapshot for devices:\n");
> +	for (inx = 0; inx < count; ++inx)
> +		pr_info("\t%u:%u\n", dev_id_array[inx].mj,
> +			dev_id_array[inx].mn);
> +
> +	ret = check_same_devices(dev_id_array, count);
> +	if (ret)
> +		return ret;
> +
> +	snapshot = snapshot_new(count);
> +	if (IS_ERR(snapshot)) {
> +		pr_err("Unable to create snapshot: failed to allocate snapshot structure\n");
> +		return PTR_ERR(snapshot);
> +	}
> +
> +	ret = -ENODEV;
> +	for (inx = 0; inx < count; ++inx) {
> +		dev_t dev_id =
> +			MKDEV(dev_id_array[inx].mj, dev_id_array[inx].mn);
> +		struct tracker *tracker;
> +
> +		tracker = tracker_create_or_get(dev_id);
> +		if (IS_ERR(tracker)) {
> +			pr_err("Unable to create snapshot\n");
> +			pr_err("Failed to add device [%u:%u] to snapshot tracking\n",
> +			       MAJOR(dev_id), MINOR(dev_id));
> +			ret = PTR_ERR(tracker);
> +			goto fail;
> +		}
> +
> +		snapshot->tracker_array[inx] = tracker;
> +		snapshot->count++;
> +	}
> +
> +	down_write(&snapshots_lock);
> +	list_add_tail(&snapshots, &snapshot->link);
> +	up_write(&snapshots_lock);

Given list_for_each_entry below, typo wrt &snapshots found in the fresh 2023?

> +
> +	uuid_copy(id, &snapshot->id);
> +	pr_info("Snapshot %pUb was created\n", &snapshot->id);
> +	return 0;
> +fail:
> +	pr_err("Snapshot cannot be created\n");
> +
> +	snapshot_put(snapshot);
> +	return ret;
> +}
> +
> +static struct snapshot *snapshot_get_by_id(uuid_t *id)
> +{
> +	struct snapshot *snapshot = NULL;
> +	struct snapshot *s;
> +
> +	down_read(&snapshots_lock);
> +	if (list_empty(&snapshots))
> +		goto out;
> +
> +	list_for_each_entry(s, &snapshots, link) {
> +		if (uuid_equal(&s->id, id)) {
> +			snapshot = s;
> +			snapshot_get(snapshot);
> +			break;
> +		}
> +	}
> +out:
> +	up_read(&snapshots_lock);
> +	return snapshot;
> +}

Re: [PATCH v2 17/21] block, blksnap: snapshot image block device

[Sergei Shtepa]

On 1/1/23 08:18, Hillf Danton wrote:
> Subject:
> Re: [PATCH v2 17/21] block, blksnap: snapshot image block device
> From:
> Hillf Danton <hdanton@sina.com>
> Date:
> 1/1/23, 08:18
> 
> To:
> Sergei Shtepa <sergei.shtepa@veeam.com>
> CC:
> "axboe@kernel.dk" <axboe@kernel.dk>, "corbet@lwn.net" <corbet@lwn.net>, "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
> 
> 
> This is the first time you've received an email from this sender 
> hdanton@sina.com, please exercise caution when clicking on links or opening 
> attachments.
> 
> 
> On 9 Dec 2022 15:23:27 +0100 Sergei Shtepa
>  > Provides the operation of block devices of snapshot images. Read and
>  > write operations are redirected to the regions of difference blocks for
>  > block device (struct diff_area).
>  >
>  > Signed-off-by: Sergei Shtepa
>  > ---
> 
> Thanks for your patchset.

Thanks for the review.

> 
>  > +static int snapimage_kthread_worker_fn(void *param)
>  > +{
>  > + struct snapimage *snapimage = param;
>  > + struct bio *bio;
>  > + int ret = 0;
>  > +
>  > + while (!kthread_should_stop()) {
>  > + bio = get_bio_from_queue(snapimage);
>  > + if (!bio) {
>  > + schedule_timeout_interruptible(HZ / 100);
>  > + continue;
>  > + }
> 
> Given the wake_up_process() below, s$HZ / 100$HZ * 1000$ to avoid
> unnecessary wakeups [1].
> 
> And because of no signal handling added, use schedule_timeout_idle() instead.

Yes, this function will be rewritten to eliminate unnecessary wake-ups.
The code that Christoph proposed in the letter at Dec 15 should work well.

> 
> [1] 
> https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20210419085027.761150-2-elver%40google.com%2F&data=05%7C01%7Csergei.shtepa%40veeam.com%7C765d591d47ad4d93857208daebc92508%7Cba07baab431b49edadd7cbc3542f5140%7C1%7C1%7C638081546440884849%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=rbINOq7m31DFhjHLnQKFHQuoWB64ZhmnWbAVLuNwv7w%3D&reserved=0
> 
>  > +
>  > + snapimage_process_bio(snapimage, bio);
>  > + }
>  > +
>  > + while ((bio = get_bio_from_queue(snapimage)))
>  > + snapimage_process_bio(snapimage, bio);
>  > +
>  > + return ret;
>  > +}
>  > +
>  > +static void snapimage_submit_bio(struct bio *bio)
>  > +{
>  > + struct snapimage *snapimage = bio->bi_bdev->bd_disk->private_data;
>  > + gfp_t gfp = GFP_NOIO;
>  > +
>  > + if (bio->bi_opf & REQ_NOWAIT)
>  > + gfp |= GFP_NOWAIT;
>  > + if (snapimage->is_ready) {
>  > + spin_lock(&snapimage->queue_lock);
>  > + bio_list_add(&snapimage->queue, bio);
>  > + spin_unlock(&snapimage->queue_lock);
>  > +
>  > + wake_up_process(snapimage->worker);
>  > + } else
>  > + bio_io_error(bio);
>  > +}
> 

Re: [PATCH v2 18/21] block, blksnap: snapshot

[Sergei Shtepa]

On 1/1/23 12:05, Hillf Danton wrote:
> Subject:
> Re: [PATCH v2 18/21] block, blksnap: snapshot
> From:
> Hillf Danton <hdanton@sina.com>
> Date:
> 1/1/23, 12:05
> 
> To:
> Sergei Shtepa <sergei.shtepa@veeam.com>
> CC:
> axboe@kernel.dk, corbet@lwn.net, linux-mm@kvack.org, linux-kernel@vger.kernel.org
> 
> 
> On 9 Dec 2022 15:23:28 +0100 Sergei Shtepa <sergei.shtepa@veeam.com>
>> +int snapshot_create(struct blk_snap_dev *dev_id_array, unsigned int count,
>> +		    uuid_t *id)
>> +{
>> +	struct snapshot *snapshot = NULL;
>> +	int ret;
>> +	unsigned int inx;
>> +
>> +	pr_info("Create snapshot for devices:\n");
>> +	for (inx = 0; inx < count; ++inx)
>> +		pr_info("\t%u:%u\n", dev_id_array[inx].mj,
>> +			dev_id_array[inx].mn);
>> +
>> +	ret = check_same_devices(dev_id_array, count);
>> +	if (ret)
>> +		return ret;
>> +
>> +	snapshot = snapshot_new(count);
>> +	if (IS_ERR(snapshot)) {
>> +		pr_err("Unable to create snapshot: failed to allocate snapshot structure\n");
>> +		return PTR_ERR(snapshot);
>> +	}
>> +
>> +	ret = -ENODEV;
>> +	for (inx = 0; inx < count; ++inx) {
>> +		dev_t dev_id =
>> +			MKDEV(dev_id_array[inx].mj, dev_id_array[inx].mn);
>> +		struct tracker *tracker;
>> +
>> +		tracker = tracker_create_or_get(dev_id);
>> +		if (IS_ERR(tracker)) {
>> +			pr_err("Unable to create snapshot\n");
>> +			pr_err("Failed to add device [%u:%u] to snapshot tracking\n",
>> +			       MAJOR(dev_id), MINOR(dev_id));
>> +			ret = PTR_ERR(tracker);
>> +			goto fail;
>> +		}
>> +
>> +		snapshot->tracker_array[inx] = tracker;
>> +		snapshot->count++;
>> +	}
>> +
>> +	down_write(&snapshots_lock);
>> +	list_add_tail(&snapshots, &snapshot->link);
>> +	up_write(&snapshots_lock);
> Given list_for_each_entry below, typo wrt &snapshots found in the fresh 2023?
> 

Thanks.
It seems I just swapped the parameters by mistake.
It should be better:
```
	down_write(&snapshots_lock);
	list_add_tail(&snapshot->link, &snapshots);
	up_write(&snapshots_lock);
```

>> +
>> +	uuid_copy(id, &snapshot->id);
>> +	pr_info("Snapshot %pUb was created\n", &snapshot->id);
>> +	return 0;
>> +fail:
>> +	pr_err("Snapshot cannot be created\n");
>> +
>> +	snapshot_put(snapshot);
>> +	return ret;
>> +}
>> +
>> +static struct snapshot *snapshot_get_by_id(uuid_t *id)
>> +{
>> +	struct snapshot *snapshot = NULL;
>> +	struct snapshot *s;
>> +
>> +	down_read(&snapshots_lock);
>> +	if (list_empty(&snapshots))
>> +		goto out;
>> +
>> +	list_for_each_entry(s, &snapshots, link) {
>> +		if (uuid_equal(&s->id, id)) {
>> +			snapshot = s;
>> +			snapshot_get(snapshot);
>> +			break;
>> +		}
>> +	}
>> +out:
>> +	up_read(&snapshots_lock);
>> +	return snapshot;
>> +}