Re: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE

Re: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE

[Ming Lei]

Hello Jared,

On Thu, Apr 03, 2025 at 12:37:11PM +0000, Jared Holzman wrote:
> Apologies if this is a dup, but I am not seeing the original mail on the mailing list archive.

I guess it is because the patch is sent as html, instead of plain test,
please follow the patch submission guide:

https://www.kernel.org/doc/Documentation/process/submitting-patches.rst

> 
> ________________________________
> From: Jared Holzman <jholzman@nvidia.com>
> Sent: Monday, 31 March 2025 4:54 PM
> To: linux-block@vger.kernel.org <linux-block@vger.kernel.org>
> Cc: ming.lei@redhat.com <ming.lei@redhat.com>; Omri Mann <omri@nvidia.com>; Ofer Oshri <ofer@nvidia.com>; Omri Levi <omril@nvidia.com>; Jared Holzman <jholzman@nvidia.com>
> Subject: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE
> 
> From: Omri Mann <omri@nvidia.com>
> 
> Currently ublk only allows the size of the ublkb block device to be
> set via UBLK_CMD_SET_PARAMS before UBLK_CMD_START_DEV is triggered.
> 
> This does not provide support for extendable user-space block devices
> without having to stop and restart the underlying ublkb block device
> causing IO interruption.

The requirement is reasonable.

> 
> This patch adds a new ublk command UBLK_U_CMD_SET_SIZE to allow the
> ublk block device to be resized on-the-fly.

Looks CMD_SET_SIZE is not generic enough, maybe UBLK_CMD_UPDATE_PARAMS
can be added for support any parameter update by allowing to do it
when device is in LIVE state.

> 
> Signed-off-by: Omri Mann <omri@nvidia.com>
> ---
>  Documentation/block/ublk.rst  |  5 +++++
>  drivers/block/ublk_drv.c      | 26 +++++++++++++++++++++++++-
>  include/uapi/linux/ublk_cmd.h |  7 +++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
> index 1e0e7358e14a..7eca87a66b9c 100644
> --- a/Documentation/block/ublk.rst
> +++ b/Documentation/block/ublk.rst
> @@ -139,6 +139,11 @@ managing and controlling ublk devices with help of several control commands:
>    set up the per-queue context efficiently, such as bind affine CPUs with IO
>    pthread and try to allocate buffers in IO thread context.
> 
> +- ``UBLK_F_SET_SIZE``
> +
> +  Allows changing the size of the block device after it has started. Useful for
> +  userspace implementations that allow extending the underlying block device.
> +
>  - ``UBLK_CMD_GET_DEV_INFO``
> 
>    For retrieving device info via ``ublksrv_ctrl_dev_info``. It is the server's
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index c060da409ed8..ab6364475b9c 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -64,7 +64,8 @@
>                  | UBLK_F_CMD_IOCTL_ENCODE \
>                  | UBLK_F_USER_COPY \
>                  | UBLK_F_ZONED \
> -               | UBLK_F_USER_RECOVERY_FAIL_IO)
> +               | UBLK_F_USER_RECOVERY_FAIL_IO \
> +               | UBLK_F_SET_SIZE)
> 
>  #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
>                  | UBLK_F_USER_RECOVERY_REISSUE \
> @@ -2917,6 +2918,25 @@ static int ublk_ctrl_get_features(struct io_uring_cmd *cmd)
>          return 0;
>  }
> 
> +static int ublk_ctrl_set_size(struct ublk_device *ub,
> +               struct io_uring_cmd *cmd)
> +{
> +       const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
> +       struct ublk_param_basic *p = &ub->params.basic;
> +       size_t new_size = (int)header->data[0];
> +       int ret = 0;
> +       unsigned int memflags;
> +
> +       p->dev_sectors = new_size;
> +
> +       memflags = blk_mq_freeze_queue(ub->ub_disk->queue);
> +       mutex_lock(&ub->mutex);
> +       set_capacity_and_notify(ub->ub_disk, p->dev_sectors);
> +       mutex_unlock(&ub->mutex);
> +       blk_mq_unfreeze_queue(ub->ub_disk->queue, memflags);

Actually if it is just for updating device size, queue freeze isn't needed,
because bio_check_eod() is called without grabbing ->q_usage_counter, but
for updating other parameters, freezing queue is often needed.


Thanks,
Ming

Re: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE

[Jared Holzman]

Hi Ming,

On 05/04/2025 5:51, Ming Lei wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Jared,
>
> On Thu, Apr 03, 2025 at 12:37:11PM +0000, Jared Holzman wrote:
>> Apologies if this is a dup, but I am not seeing the original mail on the mailing list archive.
> I guess it is because the patch is sent as html, instead of plain test,
> please follow the patch submission guide:
>
> https://www.kernel.org/doc/Documentation/process/submitting-patches.rst

Sorry about that, I originally sent the mail using git send-mail, but 
our internal smtp relay does not support outside addresses. I then tried 
forwarding it from Outlook and it decided to add HTML without telling me.

I'm using Thunderbird now, so hopefully it will be in plain-text as 
required.

>> ________________________________
>> From: Jared Holzman <jholzman@nvidia.com>
>> Sent: Monday, 31 March 2025 4:54 PM
>> To: linux-block@vger.kernel.org <linux-block@vger.kernel.org>
>> Cc: ming.lei@redhat.com <ming.lei@redhat.com>; Omri Mann <omri@nvidia.com>; Ofer Oshri <ofer@nvidia.com>; Omri Levi <omril@nvidia.com>; Jared Holzman <jholzman@nvidia.com>
>> Subject: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE
>>
>> From: Omri Mann <omri@nvidia.com>
>>
>> Currently ublk only allows the size of the ublkb block device to be
>> set via UBLK_CMD_SET_PARAMS before UBLK_CMD_START_DEV is triggered.
>>
>> This does not provide support for extendable user-space block devices
>> without having to stop and restart the underlying ublkb block device
>> causing IO interruption.
> The requirement is reasonable.
>
>> This patch adds a new ublk command UBLK_U_CMD_SET_SIZE to allow the
>> ublk block device to be resized on-the-fly.
> Looks CMD_SET_SIZE is not generic enough, maybe UBLK_CMD_UPDATE_PARAMS
> can be added for support any parameter update by allowing to do it
> when device is in LIVE state.

That's fine, but we'd rather not take on the burden of verifying all of 
ublk_params to see which ones can be safely changed on-the-fly.

Would it be reasonable to have UBLK_CMD_UPDATE_PARAMS accept a different 
struct "ublk_param_update" which contains only the parameters that can 
be updated in the LIVE state and will include only max_sectors for now?

Alternatively if you know off the top of your head which parameters can 
be easily changed on-the-fly and we will add only those.

>> Signed-off-by: Omri Mann <omri@nvidia.com>
>> ---
>>   Documentation/block/ublk.rst  |  5 +++++
>>   drivers/block/ublk_drv.c      | 26 +++++++++++++++++++++++++-
>>   include/uapi/linux/ublk_cmd.h |  7 +++++++
>>   3 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
>> index 1e0e7358e14a..7eca87a66b9c 100644
>> --- a/Documentation/block/ublk.rst
>> +++ b/Documentation/block/ublk.rst
>> @@ -139,6 +139,11 @@ managing and controlling ublk devices with help of several control commands:
>>     set up the per-queue context efficiently, such as bind affine CPUs with IO
>>     pthread and try to allocate buffers in IO thread context.
>>
>> +- ``UBLK_F_SET_SIZE``
>> +
>> +  Allows changing the size of the block device after it has started. Useful for
>> +  userspace implementations that allow extending the underlying block device.
>> +
>>   - ``UBLK_CMD_GET_DEV_INFO``
>>
>>     For retrieving device info via ``ublksrv_ctrl_dev_info``. It is the server's
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index c060da409ed8..ab6364475b9c 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -64,7 +64,8 @@
>>                   | UBLK_F_CMD_IOCTL_ENCODE \
>>                   | UBLK_F_USER_COPY \
>>                   | UBLK_F_ZONED \
>> -               | UBLK_F_USER_RECOVERY_FAIL_IO)
>> +               | UBLK_F_USER_RECOVERY_FAIL_IO \
>> +               | UBLK_F_SET_SIZE)
>>
>>   #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
>>                   | UBLK_F_USER_RECOVERY_REISSUE \
>> @@ -2917,6 +2918,25 @@ static int ublk_ctrl_get_features(struct io_uring_cmd *cmd)
>>           return 0;
>>   }
>>
>> +static int ublk_ctrl_set_size(struct ublk_device *ub,
>> +               struct io_uring_cmd *cmd)
>> +{
>> +       const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
>> +       struct ublk_param_basic *p = &ub->params.basic;
>> +       size_t new_size = (int)header->data[0];
>> +       int ret = 0;
>> +       unsigned int memflags;
>> +
>> +       p->dev_sectors = new_size;
>> +
>> +       memflags = blk_mq_freeze_queue(ub->ub_disk->queue);
>> +       mutex_lock(&ub->mutex);
>> +       set_capacity_and_notify(ub->ub_disk, p->dev_sectors);
>> +       mutex_unlock(&ub->mutex);
>> +       blk_mq_unfreeze_queue(ub->ub_disk->queue, memflags);
> Actually if it is just for updating device size, queue freeze isn't needed,
> because bio_check_eod() is called without grabbing ->q_usage_counter, but
> for updating other parameters, freezing queue is often needed.

Gotcha. I will remove it and send a new version of the patch.

Thanks,

Jared

>
>
> Thanks,
> Ming
>
-- 
Jared Holzman
Senior Software Engineer
NVIDIA
jholzman@nvidia.com

Re: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE

[Ming Lei]

On Sun, Apr 06, 2025 at 11:17:05PM +0300, Jared Holzman wrote:
> Hi Ming,
> 
> On 05/04/2025 5:51, Ming Lei wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hello Jared,
> > 
> > On Thu, Apr 03, 2025 at 12:37:11PM +0000, Jared Holzman wrote:
> > > Apologies if this is a dup, but I am not seeing the original mail on the mailing list archive.
> > I guess it is because the patch is sent as html, instead of plain test,
> > please follow the patch submission guide:
> > 
> > https://www.kernel.org/doc/Documentation/process/submitting-patches.rst
> 
> Sorry about that, I originally sent the mail using git send-mail, but our
> internal smtp relay does not support outside addresses. I then tried
> forwarding it from Outlook and it decided to add HTML without telling me.
> 
> I'm using Thunderbird now, so hopefully it will be in plain-text as
> required.
> 
> > > ________________________________
> > > From: Jared Holzman <jholzman@nvidia.com>
> > > Sent: Monday, 31 March 2025 4:54 PM
> > > To: linux-block@vger.kernel.org <linux-block@vger.kernel.org>
> > > Cc: ming.lei@redhat.com <ming.lei@redhat.com>; Omri Mann <omri@nvidia.com>; Ofer Oshri <ofer@nvidia.com>; Omri Levi <omril@nvidia.com>; Jared Holzman <jholzman@nvidia.com>
> > > Subject: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE
> > > 
> > > From: Omri Mann <omri@nvidia.com>
> > > 
> > > Currently ublk only allows the size of the ublkb block device to be
> > > set via UBLK_CMD_SET_PARAMS before UBLK_CMD_START_DEV is triggered.
> > > 
> > > This does not provide support for extendable user-space block devices
> > > without having to stop and restart the underlying ublkb block device
> > > causing IO interruption.
> > The requirement is reasonable.
> > 
> > > This patch adds a new ublk command UBLK_U_CMD_SET_SIZE to allow the
> > > ublk block device to be resized on-the-fly.
> > Looks CMD_SET_SIZE is not generic enough, maybe UBLK_CMD_UPDATE_PARAMS
> > can be added for support any parameter update by allowing to do it
> > when device is in LIVE state.
> 
> That's fine, but we'd rather not take on the burden of verifying all of
> ublk_params to see which ones can be safely changed on-the-fly.
> 
> Would it be reasonable to have UBLK_CMD_UPDATE_PARAMS accept a different
> struct "ublk_param_update" which contains only the parameters that can be
> updated in the LIVE state and will include only max_sectors for now?
> 
> Alternatively if you know off the top of your head which parameters can be
> easily changed on-the-fly and we will add only those.
 
Fair enough, updating 'dev_sectors' should be generic enough, and it looks
fine to add UBLK_U_CMD_SET_SIZE.

In future, if there is requirement for updating other parameters, we can
add UBLK_U_CMD_UPDATE_PARAMS, and most of parameters should be allowed to
update.


Thanks,
Ming

Re: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE

[Ming Lei]

On Mon, Apr 07, 2025 at 11:14:34AM +0800, Ming Lei wrote:
> On Sun, Apr 06, 2025 at 11:17:05PM +0300, Jared Holzman wrote:
> > Hi Ming,
> > 
> > On 05/04/2025 5:51, Ming Lei wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > Hello Jared,
> > > 
> > > On Thu, Apr 03, 2025 at 12:37:11PM +0000, Jared Holzman wrote:
> > > > Apologies if this is a dup, but I am not seeing the original mail on the mailing list archive.
> > > I guess it is because the patch is sent as html, instead of plain test,
> > > please follow the patch submission guide:
> > > 
> > > https://www.kernel.org/doc/Documentation/process/submitting-patches.rst
> > 
> > Sorry about that, I originally sent the mail using git send-mail, but our
> > internal smtp relay does not support outside addresses. I then tried
> > forwarding it from Outlook and it decided to add HTML without telling me.
> > 
> > I'm using Thunderbird now, so hopefully it will be in plain-text as
> > required.
> > 
> > > > ________________________________
> > > > From: Jared Holzman <jholzman@nvidia.com>
> > > > Sent: Monday, 31 March 2025 4:54 PM
> > > > To: linux-block@vger.kernel.org <linux-block@vger.kernel.org>
> > > > Cc: ming.lei@redhat.com <ming.lei@redhat.com>; Omri Mann <omri@nvidia.com>; Ofer Oshri <ofer@nvidia.com>; Omri Levi <omril@nvidia.com>; Jared Holzman <jholzman@nvidia.com>
> > > > Subject: [PATCH] ublk: Add UBLK_U_CMD_SET_SIZE
> > > > 
> > > > From: Omri Mann <omri@nvidia.com>
> > > > 
> > > > Currently ublk only allows the size of the ublkb block device to be
> > > > set via UBLK_CMD_SET_PARAMS before UBLK_CMD_START_DEV is triggered.
> > > > 
> > > > This does not provide support for extendable user-space block devices
> > > > without having to stop and restart the underlying ublkb block device
> > > > causing IO interruption.
> > > The requirement is reasonable.
> > > 
> > > > This patch adds a new ublk command UBLK_U_CMD_SET_SIZE to allow the
> > > > ublk block device to be resized on-the-fly.
> > > Looks CMD_SET_SIZE is not generic enough, maybe UBLK_CMD_UPDATE_PARAMS
> > > can be added for support any parameter update by allowing to do it
> > > when device is in LIVE state.
> > 
> > That's fine, but we'd rather not take on the burden of verifying all of
> > ublk_params to see which ones can be safely changed on-the-fly.
> > 
> > Would it be reasonable to have UBLK_CMD_UPDATE_PARAMS accept a different
> > struct "ublk_param_update" which contains only the parameters that can be
> > updated in the LIVE state and will include only max_sectors for now?
> > 
> > Alternatively if you know off the top of your head which parameters can be
> > easily changed on-the-fly and we will add only those.
>  
> Fair enough, updating 'dev_sectors' should be generic enough, and it looks
> fine to add UBLK_U_CMD_SET_SIZE.

Also UBLK_U_CMD_SET_SIZE may be renamed as UBLK_U_CMD_UPDATE_SIZE.


Thanks,
Ming