Hi Jevon,
On Sun, 10 Apr 2016 14:55:55 +0800
Jevon Qiao <scaleqiao@xxxxxxxxx> wrote:
> Hi Greg,
>
> Thank you for spending time reviewing this patch.
> On 7/4/16 23:50, Greg Kurz wrote:
> > On Tue, 15 Mar 2016 00:02:48 +0800
> > Jevon Qiao <scaleqiao@xxxxxxxxx> wrote:
> >
> >> Ceph as a promising unified distributed storage system is widely used in the
> >> world of OpenStack. OpenStack users deploying Ceph for block (Cinder) and
> >> object (S3/Swift) are unsurprisingly looking at Manila and CephFS to round out
> >> a unified storage solution. Since the typical hypervisor people are using is
> >> Qemu/KVM, it is necessary to provide a high performance, easy to use, file
> >> system service in it. VirtFS aims to offers paravirtualized system services and
> >> simple passthrough for directories from host to guest, which currently only
> >> support local file system, this patch wants to add CephFS support in VirtFS.
> >>
> >> Signed-off-by: Jevon Qiao <scaleqiao@xxxxxxxxx>
> >> ---
> > Jevon,
> >
> > There's still work to be done on this patch.
> >
> > One general remark is that there are far too many traces: it obfuscates the code
> > and does not bring much benefit in my opinion. If you look at the other fsdev
> > drivers, you see they don't do traces at all !
> > Also, I've found several errors where the code simply cannot work... please run
> > a file/io oriented testsuite in the guest to check all the fsdev operations are
> > working as expected... maybe some tests from LTP ?
Please leave at least one empty line before...
> Well, I did run some tests against this patch with iozone to guarantee
> the IO path is correct and did some manual tests to make sure that the
> basic file/directory operations work. Anyway, I will run the file system
> related parts of LTP.
... and after your comments, so that we see them well.
Unless I've missed something, you had only one question.
> [...]
> >> +#ifndef HAVE_CEPH_READV
> >> +static ssize_t ceph_preadv(struct ceph_mount_info *cmount, int fd,
> >> + const struct iovec *iov, int iov_cnt,
> >> + off_t offset)
> >> +{
> >> + ssize_t ret;
> >> + size_t i;
> >> + size_t len, tmp;
> >> + void *buf;
> >> + size_t bufoffset = 0;
> >> +
> >> + len = iov_size(iov, iov_cnt);
> >> + buf = g_new0(uint8_t, len);
> >> + ret = ceph_read(cmount, fd, buf, len, offset);
> >> + if (ret < 0) {
> >> + return ret;
> > buf is leaked.
> >
> >> + } else {
> >> + tmp = ret;
> >> + for (i = 0; (i < iov_cnt && tmp > 0); i++) {
> > tmp is <= to the sum of all iov_len: unless I miss something, it
> > isn't possible for i to reach iov_cnt. Also the parenthesis aren't
> > needed.
> So do you mean only tmp>0 is needed here?
That's my point, yes.
Cheers.
--
Greg
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
