All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maged Mokhtar <mmokhtar@petasan.org>
To: Ilya Dryomov <idryomov@gmail.com>
Cc: Ceph Development <ceph-devel@vger.kernel.org>,
	Alex Elder <elder@linaro.org>, David Disseldorp <ddiss@suse.de>
Subject: Re: wip-fancy-striping in ceph/ceph-client
Date: Fri, 16 Feb 2018 18:30:11 +0200	[thread overview]
Message-ID: <acdbe5717d29dccdb7dae03696204886@petasan.org> (raw)
In-Reply-To: <CAOi1vP-4_gmH2DqL2EN2_MkLju3frqBkbun7P=U=HrMUMLnOgw@mail.gmail.com>

On 2018-02-13 11:58, Ilya Dryomov wrote:

> Hello,
> 
> I've pushed wip-fancy-striping with my striping v2 patches.  I didn't
> want to patch bomb the list and opened a dummy PR for feedback:
> 
> https://github.com/ceph/ceph-client/pull/19
> 
> As this is a full striping v2 implementation with adjacent extent
> merging and no limitations on parent layouts, the patch set is very
> invasive -- it's basically a rewrite of the entire I/O path.  Any
> review comments and testing cycles would be greatly appreciated!
> 
> Thanks,
> 
> Ilya

Hi Ilya,

I looked at the patches, it is not a small change but is very nicely 
done. i was surprised since even though you added complex feature 
support, the new code is less (in rbd.c) and is clearer. The different 
fill from functions and different osd request creation functions is a 
very clean design.

If i may say any minor/cosmetic comments:

Out of all the old code i was happy to see removed, the only thing i 
would "miss" is the obj_requests list in the rbd_img_request struct. It 
is not needed now, but i tend to think it is logical to have a place to 
find all osd requests for a particular top block layer request. it maybe 
just a warm a and fuzzy feeling to have this control, but it could be 
useful for debugging or if in the future we think of implementing a task 
abort operation and need to find the osd inflight ops in a quick way.

Another cosmetic thing, i tend to think the new object <-> file extent 
conversion be placed in a separate new file rather than osdmap.c

I would not be able to do full test any time soon, but i do intend to do 
some manual testing of basic functionality next week. i have in mind 
writing data pattern with each value repeating the size of stripe unit 
and doing a mix of read/write interchangeably between rbd and librbd, i 
would also test copyup functionality in clones. I will let you know if i 
do find any issues.

Cheers
Maged

  parent reply	other threads:[~2018-02-16 16:51 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-13  9:58 wip-fancy-striping in ceph/ceph-client Ilya Dryomov
2018-02-13 15:05 ` Alex Elder
2018-02-13 16:24   ` Ilya Dryomov
2018-02-13 16:38     ` Alex Elder
2018-02-16 16:30 ` Maged Mokhtar [this message]
2018-02-17  8:44   ` Ilya Dryomov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=acdbe5717d29dccdb7dae03696204886@petasan.org \
    --to=mmokhtar@petasan.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=ddiss@suse.de \
    --cc=elder@linaro.org \
    --cc=idryomov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.