[Iscsitarget-devel] struct tio reform

2 views
Skip to first unread message

Yucong Sun (叶雨飞)

unread,
Mar 27, 2012, 1:01:06 AM3/27/12
to iscsitarget-devel, Ross S. W. Walker
Hi,

After reading the code mulitple times I think I finally figured out
how tio works, but I don't understand the reason why it is being
written in this way, the definition is in following:

struct tio {
u32 pg_cnt;

u32 idx;
u32 offset;
u32 size;
struct page **pvec;

atomic_t count;
};

1) tio->idx and tio->offset are actually computed by LBA offset /
PAGE_CACHE_SIZE and LBA offset % PAGE_CACHE_SIZE , and when actually
using tio_write tio_read, the underlaying io system have to convert
them back to get the real LBA offset. This is not only unnecessary
but also very confusing, we could simply store the LBA offset directly
in the first place.

2) pvec hold a bunch of pages where suppose to store the io data,
either write or read. Naturally you would think that data always
starts at page_address(pvec[0]) , however it is not always the case,
tio->offset is also in the middle where it's possible that your data
actually starts at page 0 + tio->offset , which is again useless and
confusing, leaving some holes and every one using them will have to
compensate as well.

3) for reasons above, right now one would need both LBA and size of
the IO to calculate the page numbers you need, get_pgcnt(length,
offset) , this is totally unnecessary if we simply say the data always
start at offset 0 in tio's page_cache.

So, for reasons above, I propose to simplify the code by change struct
tio to this:

/* target io */
struct tio {
u32 pg_cnt; /* total page count */

loff_t offset; /* byte offset on target */
u32 size; /* total io bytes */
struct page **pvec; /* array of pages holding data */

atomic_t count; /* ref count */
};

The data is always stored in offset 0 in pages in pvec, no idx anymore
and get_pgcnt only needs length. I will be sending another patch
shortly unless anyone objects.

------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
Iscsitarget-devel mailing list
Iscsitar...@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iscsitarget-devel

Yucong Sun (叶雨飞)

unread,
Mar 27, 2012, 2:36:01 AM3/27/12
to iscsitarget-devel, Ross S. W. Walker
Please merge http://pastebin.com/FfL0Hqgf

This change has been tested by running badblocks over the target disk
multiple times and all blocks works fine.

Cheers.

Ross S. W. Walker

unread,
Mar 27, 2012, 8:17:14 AM3/27/12
to Yucong Sun (叶雨飞), iscsitarget-devel
A good test i think is writing an old DOS MBR and a FAT partition and format it.

Then make sure it starts at the sector it says it does (63) and ends where it says it does (last cylinder boundary, whatever that may be these days).

-Ross

This e-mail, and any attachments thereto, is intended only for use by the addressee(s) named herein and may contain legally privileged and/or confidential information. If you are not the intended recipient of this e-mail, you are hereby notified that any dissemination, distribution or copying of this e-mail, and any attachments thereto, is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender and permanently delete the original and any copy or printout thereof.
Reply all
Reply to author
Forward
0 new messages