On Fri, Mar 31, 2017 at 2:38 PM, Fam Zheng <f...@redhat.com> wrote:
> On Fri, 03/31 14:17, Ashijeet Acharya wrote:
>> On Fri, Mar 31, 2017 at 12:56 PM, Fam Zheng <f...@redhat.com> wrote:
>> > On Sat, 03/25 16:48, Ashijeet Acharya wrote:
>> >> Include a next pointer in VmdkMetaData struct to point to the previous
>> >> allocated L2 table. Modify vmdk_L2update to start updating metadata for
>> >> allocation of multiple clusters at once.
>> >>
>> >> Signed-off-by: Ashijeet Acharya <ashijeetacha...@gmail.com>
>> >> ---
>> >> block/vmdk.c | 131
>> >> ++++++++++++++++++++++++++++++++++++++++++++++-------------
>> >> 1 file changed, 102 insertions(+), 29 deletions(-)
>> >>
>> >> diff --git a/block/vmdk.c b/block/vmdk.c
>> >> index 3de8b8f..4517409 100644
>> >> --- a/block/vmdk.c
>> >> +++ b/block/vmdk.c
>> >> @@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
>> >> int valid;
>> >> uint32_t *l2_cache_entry;
>> >> uint32_t nb_clusters;
>> >> + uint32_t offset;
>> >> + struct VmdkMetaData *next;
>> >> } VmdkMetaData;
>> >>
>> >> typedef struct VmdkGrainMarker {
>> >> @@ -1037,29 +1039,81 @@ static void vmdk_refresh_limits(BlockDriverState
>> >> *bs, Error **errp)
>> >> }
>> >> }
>> >>
>> >> -static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> >> - uint32_t offset)
>> >> +static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
>> >> + VmdkMetaData *m_data, bool zeroed)
>> >> {
>> >> - offset = cpu_to_le32(offset);
>> >> + int i;
>> >> + uint32_t offset, temp_offset;
>> >> +
>> >> + if (zeroed) {
>> >> + temp_offset = VMDK_GTE_ZEROED;
>> >> + } else {
>> >> + temp_offset = m_data->offset;
>> >> + }
>> >> +
>> >> + temp_offset = cpu_to_le32(temp_offset);
>> >> +
>> >> /* update L2 table */
>> >> - if (bdrv_pwrite_sync(extent->file,
>> >> + offset = temp_offset;
>> >> + for (i = 0; i < m_data->nb_clusters; i++) {
>> >> + if (bdrv_pwrite_sync(extent->file,
>> >> ((int64_t)m_data->l2_offset * 512)
>> >> - + (m_data->l2_index * sizeof(offset)),
>> >> - &offset, sizeof(offset)) < 0) {
>> >> - return VMDK_ERROR;
>> >> + + ((m_data->l2_index + i) * sizeof(offset)),
>> >> + &(offset), sizeof(offset)) < 0) {
>> >> + return VMDK_ERROR;
>> >> + }
>> >> + if (!zeroed) {
>> >> + offset += 128;
>> >> + }
>> >> }
>> >> +
>> >> /* update backup L2 table */
>> >> + offset = temp_offset;
>> >> if (extent->l1_backup_table_offset != 0) {
>> >> m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
>> >> - if (bdrv_pwrite_sync(extent->file,
>> >> - ((int64_t)m_data->l2_offset * 512)
>> >> - + (m_data->l2_index * sizeof(offset)),
>> >> - &offset, sizeof(offset)) < 0) {
>> >> - return VMDK_ERROR;
>> >> + for (i = 0; i < m_data->nb_clusters; i++) {
>> >> + if (bdrv_pwrite_sync(extent->file,
>> >> + ((int64_t)m_data->l2_offset * 512)
>> >> + + ((m_data->l2_index + i) * sizeof(offset)),
>> >> + &(offset), sizeof(offset)) < 0) {
>> >> + return VMDK_ERROR;
>> >> + }
>> >> + if (!zeroed) {
>> >> + offset += 128;
>> >> + }
>> >> }
>> >> }
>> >> +
>> >> + offset = temp_offset;
>> >> if (m_data->l2_cache_entry) {
>> >> - *m_data->l2_cache_entry = offset;
>> >> + for (i = 0; i < m_data->nb_clusters; i++) {
>> >> + *m_data->l2_cache_entry = offset;
>> >> + m_data->l2_cache_entry++;
>> >> +
>> >> + if (!zeroed) {
>> >> + offset += 128;
>> >> + }
>> >> + }
>> >> + }
>> >> +
>> >> + return VMDK_OK;
>> >> +}
>> >> +
>> >> +static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> >> + bool zeroed)
>> >> +{
>> >> + int ret;
>> >> +
>> >> + while (m_data->next != NULL) {
>> >> + VmdkMetaData *next;
>> >> +
>> >> + ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
>> >> + if (ret < 0) {
>> >> + return ret;
>> >> + }
>> >> +
>> >> + next = m_data->next;
>> >> + m_data = next;
>> >
>> > I don't see why the next pointer is necessary. Also the tail is always
>> > unused,
>> > why do you need to allocate it?
>>
>> If I don't allocate the tail, I was getting a segfault in vmdk_pwritev().
>
> That may be because the way you interate the linked list in vmdk_pwritev is:
>
>> while (m_data->next != NULL) {
>> VmdkMetaData *next;
>> next = m_data->next;
>> g_free(m_data);
>> m_data = next;
>> }
>>
>
> which does require a dummy tail.
No, I remember it segfaulting even before I inserted that piece of
code. I think the reason is that I try to access m_data->valid inside
vmdk_pwritev()...
>
> But after all it's still not clear to me why you cannot keep m_data on stack,
...and by using malloc and moving it to the heap solved my problem,
plus for constructing the linked list.
> and why you need the next pointer at all.
If I don't segregate them in batches of 512, I will need to increment
the l2_offset manually...right? If I don't use the next pointer, what
solution do you recommend?
Ashijeet
>
> Fam
