| Message ID | 20220527103021.452651-1-tsahu@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ndctl/namespace: Fix and improve write-infoblock | expand |
Hi, Just a gentle reminder. May you please look into these patches. Please let me know if any changes are required. Thanks Tarun On Fri, 2022-05-27 at 16:00 +0530, Tarun Sahu wrote: > This series resolves some issues with write-infoblock > command and provide support to write-infoblock for sector > mode namespace > > write-infoblock command has issues regarding updating the > align, uuid, parent_uuid. In case of no parameter passed > for it, this command used to overwrite the existing values > with defaults. > > In PATCH 1/2 these parameters will be set to their original > values, incase, values hasn't been passed in command > arguments > > write-infoblock command doesn't have support for sector/BTT > mode namespaces. They can be converted to fsdax, but can > not be written being in sector mode. > > In PATCH 2/2, It creates a functionality which write > infoblock of Sector/BTT namespace. Currently only uuid, > parent_uuid can be updated. In future, Support for other > parameters can easily be integrated in the > functionality. > > --- > v2: > Updated the commit message (rephrasing) in patch 1/2 > Moved the ns_info struct to namespace.c from namespace.h > put the results after --- to avoid long commit message > > v3: > reformat the commit message to meet 100 column condition > > v4: > - Moved the struct ns_info definition to the beginning of > the block > - Initialized the buf of ns_info structure in ns_info_init > - Change the format of comment in code from "//" to "/**/" > - reword the commit message of patch 2/2 > > Tarun Sahu (2): > ndctl/namespace: Fix multiple issues with write-infoblock > ndctl/namespace: Implement write-infoblock for sector mode > namespaces > > ndctl/namespace.c | 314 ++++++++++++++++++++++++++++++++++---------- > -- > 1 file changed, 231 insertions(+), 83 deletions(-) >
Hi, Just a gentle reminder. May you please look into these patches. Please let me know if any changes are required. Thanks Tarun On Fri, 2022-05-27 at 16:00 +0530, Tarun Sahu wrote: > This series resolves some issues with write-infoblock > command and provide support to write-infoblock for sector > mode namespace > > write-infoblock command has issues regarding updating the > align, uuid, parent_uuid. In case of no parameter passed > for it, this command used to overwrite the existing values > with defaults. > > In PATCH 1/2 these parameters will be set to their original > values, incase, values hasn't been passed in command > arguments > > write-infoblock command doesn't have support for sector/BTT > mode namespaces. They can be converted to fsdax, but can > not be written being in sector mode. > > In PATCH 2/2, It creates a functionality which write > infoblock of Sector/BTT namespace. Currently only uuid, > parent_uuid can be updated. In future, Support for other > parameters can easily be integrated in the > functionality. > > --- > v2: > Updated the commit message (rephrasing) in patch 1/2 > Moved the ns_info struct to namespace.c from namespace.h > put the results after --- to avoid long commit message > > v3: > reformat the commit message to meet 100 column condition > > v4: > - Moved the struct ns_info definition to the beginning of > the block > - Initialized the buf of ns_info structure in ns_info_init > - Change the format of comment in code from "//" to "/**/" > - reword the commit message of patch 2/2 > > Tarun Sahu (2): > ndctl/namespace: Fix multiple issues with write-infoblock > ndctl/namespace: Implement write-infoblock for sector mode > namespaces > > ndctl/namespace.c | 314 ++++++++++++++++++++++++++++++++++---------- > -- > 1 file changed, 231 insertions(+), 83 deletions(-) >
On Fri, 2022-05-27 at 16:00 +0530, Tarun Sahu wrote: > This series resolves some issues with write-infoblock > command and provide support to write-infoblock for sector > mode namespace > > write-infoblock command has issues regarding updating the > align, uuid, parent_uuid. In case of no parameter passed > for it, this command used to overwrite the existing values > with defaults. > > In PATCH 1/2 these parameters will be set to their original > values, incase, values hasn't been passed in command > arguments > > write-infoblock command doesn't have support for sector/BTT > mode namespaces. They can be converted to fsdax, but can > not be written being in sector mode. > > In PATCH 2/2, It creates a functionality which write > infoblock of Sector/BTT namespace. Currently only uuid, > parent_uuid can be updated. In future, Support for other > parameters can easily be integrated in the > functionality. Hi Tarun, Sorry for the delay in reviewing these. From a quick look, it looks like you're adding read-modify-write functionality to the write- infoblock command - is that correct? I think the original intent of these commands was, purely as a debug/test aid, that the user would be responsible for reading the namespace if needed, and using that as a basis for writing the infoblockl if an RMW operation is desired. However for the most part, write-infoblock just creates infoblocks out of thin air, and optionally writes it to a namepspace. I'm not sure adding a read-modify-write here is really that useful, unless you have a specific use case for this sort of thing? > > --- > v2: > Updated the commit message (rephrasing) in patch 1/2 > Moved the ns_info struct to namespace.c from namespace.h > put the results after --- to avoid long commit message > > v3: > reformat the commit message to meet 100 column condition > > v4: > - Moved the struct ns_info definition to the beginning of > the block > - Initialized the buf of ns_info structure in ns_info_init > - Change the format of comment in code from "//" to "/**/" > - reword the commit message of patch 2/2 > > Tarun Sahu (2): > ndctl/namespace: Fix multiple issues with write-infoblock > ndctl/namespace: Implement write-infoblock for sector mode > namespaces > > ndctl/namespace.c | 314 ++++++++++++++++++++++++++++++++++---------- > -- > 1 file changed, 231 insertions(+), 83 deletions(-) >
Hi, > Hi Tarun, > > Sorry for the delay in reviewing these. From a quick look, it looks > like you're adding read-modify-write functionality to the write- > infoblock command - is that correct? > > I think the original intent of these commands was, purely as a > debug/test aid, that the user would be responsible for reading the > namespace if needed, and using that as a basis for writing the > infoblockl if an RMW operation is desired. However for the most part, > write-infoblock just creates infoblocks out of thin air, and > optionally > writes it to a namepspace. I'm not sure adding a read-modify-write > here > is really that useful, unless you have a specific use case for this > sort of thing? From the man page, it is not clear that, non-provided args values will get updated without user concern, and It comes as surprise to user as user didnt intend to change that. For Example, If user only want to change the value of uuid of a namespace infoblock, So They perform write-infoblock with particular uuid but didnt pass align value which they intended not to change but write-infoblock changes it to default value. This patch, read-modify-write takes care of other arguments which are not passed for the concerned namespace as part of write-infoblock by retaining the values of them. Other patch of this series, takes advantage of the above read-modify-write functionality and implement write-infoblock for sector/BTT namespaces. Which currently allows only uuid and parent_uuid updates.