At Tue, 3 Feb 2015 11:55:34 +0800, Liu Yuan wrote: > > On Mon, Feb 02, 2015 at 06:18:52PM +0900, Hitoshi Mitake wrote: > > Current sheep cannot handle a case like this: > > 1. iterate snapshot creation and let latest working VDI have VID 0xffffff > > 2. create one more snapshot > > > > (The situation can be reproduced with the below sequence: > > $ dog vdi create 00471718 1G > > $ dog vdi snapshot 00471718 (repeat 7 times) ) > > > > In this case, new VID becomes 0x000000. Current fill_vdi_info() and > > fill_vdi_info_range() cannot handle this case. It comes from the below > > two reasons: > > 1. Recent change 00ecfb24ee46f2 introduced a bug which breaks > > fill_vdi_info_range() in a case of underflow of its variable i. > > I don't yet look close into this problem, but from this description, should we > fix the bug somewhere else? because > > 1. fill_vdi_info_range() runs well in the past, so we'd better avoid to modify > it to fix the bug introduced by other patch(es). > > 2. You mentioned it was introduced by 00ecfb24ee46f2, so we might better fix > 00ecfb24ee46f2. > > It would be great if you can elaberate the bug, what it is, why breaks > fill_vdi_info_range() and if modify fill_vdi_info_range() is the only way to > fix the problem.
The problem is related to --no-share for snapshot, its design seems to have a problem. I'll redesign it and refine this patch as a part of the rework. Thanks, Hitoshi > > > 2. fill_vdi_info_range() assumes that its parameters, left and right, > > are obtained from get_vdi_bitmap_range(). get_vdi_bitmap_range() > > obtains left and right which mean the range of existing VIDs is > > [left, right), in other words, [left, right - 1]. So > > fill_vdi_info_range() starts checking from right - 1 to left. But > > it means fill_vdi_info_range() cannot check VID 0xffffff even VID > > overflow happens. So this patch lets fill_vdi_info_range() check > > from right to left, and also change callers' manner (it passes left > > and right - 1 in ordinal cases). > > > > Signed-off-by: Hitoshi Mitake <mitake.hito...@lab.ntt.co.jp> > > --- > > sheep/vdi.c | 30 +++++++++++++++++++++++------- > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > diff --git a/sheep/vdi.c b/sheep/vdi.c > > index 2889df6..3d14ccf 100644 > > --- a/sheep/vdi.c > > +++ b/sheep/vdi.c > > @@ -1404,9 +1404,11 @@ static int fill_vdi_info_range(uint32_t left, > > uint32_t right, > > ret = SD_RES_NO_MEM; > > goto out; > > } > > - for (i = right - 1; i && i >= left; i--) { > > + > > + i = right; > > + while (i >= left) { > > if (!test_bit(i, sys->vdi_inuse)) > > - continue; > > + goto next; > > > > ret = sd_read_object(vid_to_vdi_oid(i), (char *)inode, > > SD_INODE_HEADER_SIZE, 0); > > @@ -1420,10 +1422,10 @@ static int fill_vdi_info_range(uint32_t left, > > uint32_t right, > > /* Read, delete, clone on snapshots */ > > if (!vdi_is_snapshot(inode)) { > > vdi_found = true; > > - continue; > > + goto next; > > } > > if (!vdi_tag_match(iocb, inode)) > > - continue; > > + goto next; > > } else { > > /* > > * Rollback & snap create, read, delete on > > @@ -1438,6 +1440,10 @@ static int fill_vdi_info_range(uint32_t left, > > uint32_t right, > > info->vid = inode->vdi_id; > > goto out; > > } > > +next: > > + if (!i) > > + break; > > + i--; > > I don't have the whole picture yet, but just for this fragment, we change the > for loop into goto, is not so good for code clarity. > > Thanks > Yuan > > -- > sheepdog mailing list > sheepdog@lists.wpkg.org > https://lists.wpkg.org/mailman/listinfo/sheepdog -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog