[Gluster-devel] Need inputs on patch #17985

Thu Nov 30 05:31:02 UTC 2017

I think this caused regression in quick-read. On going through code, I
realized Quick-read doesn't fetch content of a file pointed by dentry in
readdirplus. Since, the patch in question prevents any lookup from
resolver, reads on the file till the duration of "entry-timeout" (a cmdline
option to fuse mount, whose default value is 1 sec) after the entry was
discovered in readdirplus will not be served by quick-read even though the
size of file is eligible to be cached. This may cause perf regression in
read heavy workloads on smallfiles. We'll be doing more testing to identify
this.

On Tue, Sep 12, 2017 at 11:31 AM, Raghavendra G <raghavendra.hg at gmail.com>
wrote:

> Update. Two more days to go for the deadline. Till now, there are no open
> issues identified against this patch.
>
> On Fri, Sep 8, 2017 at 6:54 AM, Raghavendra Gowdappa <rgowdapp at redhat.com>
> wrote:
>
>>
>>
>> ----- Original Message -----
>> > From: "FNU Raghavendra Manjunath" <rabhat at redhat.com>
>> > To: "Raghavendra Gowdappa" <rgowdapp at redhat.com>
>> > Cc: "Raghavendra G" <raghavendra.hg at gmail.com>, "Nithya Balachandran" <
>> nbalacha at redhat.com>, anoopcs at redhat.com,
>> > "Gluster Devel" <gluster-devel at gluster.org>, "Raghavendra Bhat" <
>> raghavendra at redhat.com>
>> > Sent: Thursday, September 7, 2017 6:44:51 PM
>> > Subject: Re: [Gluster-devel] Need inputs on patch #17985
>> >
>> > From snapview client perspective one important thing to note. For
>> building
>> > the context for the entry point (by default ".snaps") a explicit lookup
>> has
>> > to be done on it. The dentry for ".snaps" is not returned when readdir
>> is
>> > done on its parent directory (Not even when ls -a is done). So for
>> building
>> > the context of .snaps (in the context snapview client saves the
>> information
>> > about whether it is a real inode or virtual inode) we need a lookup.
>>
>> Since the dentry corresponding to ".snaps" is not returned, there won't
>> be an inode for this directory linked in itable. Also, glusterfs wouldn't
>> have given nodeid corresponding to ".snaps" during readdir response (as
>> dentry itself is not returned). So, kernel would do an explicit lookup
>> before doing any operation on ".snaps" (unlike for those dentries which
>> contain nodeid kernel can choose to skip a lookup) and we are safe. So,
>> #17985 is safe in its current form.
>>
>> >
>> > From snapview server perspective as well a lookup might be needed. In
>> > snapview server a glfs handle is established between the snapview server
>> > and the snapshot brick. So a inode in snapview server process contains
>> the
>> > glfs handle for the object being accessed from snapshot.  In snapview
>> > server readdirp does not build the inode context (which contains the
>> glfs
>> > handle etc) because glfs handle is returned only in lookup.
>>
>> Same argument I've given holds good for this case too. Important point to
>> note is that "there is no dentry and hence no nodeid corresponding to
>> .snaps is passed to kernel and kernel is forced to do an explicit lookup".
>>
>> >
>> > Regards,
>> > Raghavendra
>> >
>> >
>> > On Tue, Aug 29, 2017 at 12:53 AM, Raghavendra Gowdappa <
>> rgowdapp at redhat.com>
>> > wrote:
>> >
>> > >
>> > >
>> > > ----- Original Message -----
>> > > > From: "Raghavendra G" <raghavendra.hg at gmail.com>
>> > > > To: "Nithya Balachandran" <nbalacha at redhat.com>
>> > > > Cc: "Raghavendra Gowdappa" <rgowdapp at redhat.com>,
>> anoopcs at redhat.com,
>> > > "Gluster Devel" <gluster-devel at gluster.org>,
>> > > > raghavendra at redhat.com
>> > > > Sent: Tuesday, August 29, 2017 8:52:28 AM
>> > > > Subject: Re: [Gluster-devel] Need inputs on patch #17985
>> > > >
>> > > > On Thu, Aug 24, 2017 at 2:53 PM, Nithya Balachandran <
>> > > nbalacha at redhat.com>
>> > > > wrote:
>> > > >
>> > > > > It has been a while but iirc snapview client (loaded abt dht/tier
>> etc)
>> > > had
>> > > > > some issues when we ran tiering tests. Rafi might have more info
>> on
>> > > this -
>> > > > > basically it was expecting to find the inode_ctx populated but it
>> was
>> > > not.
>> > > > >
>> > > >
>> > > > Thanks Nithya. @Rafi, @Raghavendra Bhat, is it possible to take the
>> > > > ownership of,
>> > > >
>> > > > * Identifying whether the patch in question causes the issue?
>> > >
>> > > gf_svc_readdirp_cbk is setting relevant state in inode [1]. I quickly
>> > > checked whether its the same state stored by gf_svc_lookup_cbk and it
>> looks
>> > > like the same state. So, I guess readdirp is handled correctly by
>> > > snapview-client and an explicit lookup is not required. But, will
>> wait for
>> > > inputs from rabhat and rafi.
>> > >
>> > > [1] https://github.com/gluster/glusterfs/blob/master/xlators/
>> > > features/snapview-client/src/snapview-client.c#L1962
>> > >
>> > > > * Send a fix or at least evaluate whether a fix is possible.
>> > > >
>> > > > @Others,
>> > > >
>> > > > With the motivation of getting some traction on this, Is it ok if
>> we:
>> > > > * Set a deadline of around 15 days to complete the review (or
>> testing
>> > > with
>> > > > the patch in question) of respective components and to come up with
>> > > issues
>> > > > (if any).
>> > > > * Post the deadline, if there are no open issues, go ahead and
>> merge the
>> > > > patch?
>> > > >
>> > > > If time is not enough, let us know and we can come up with a
>> reasonable
>> > > > time.
>> > > >
>> > > > regards,
>> > > > Raghavendra
>> > > >
>> > > >
>> > > > > On 24 August 2017 at 10:13, Raghavendra G <
>> raghavendra.hg at gmail.com>
>> > > > > wrote:
>> > > > >
>> > > > >> Note that we need to consider xlators on brick stack too. I've
>> added
>> > > > >> maintainers/peers of xlators on brick stack. Please explicitly
>> > > ack/nack
>> > > > >> whether this patch affects your component.
>> > > > >>
>> > > > >> For reference, following are the xlators loaded in brick stack
>> > > > >>
>> > > > >> storage/posix
>> > > > >> features/trash
>> > > > >> features/changetimerecorder
>> > > > >> features/changelog
>> > > > >> features/bitrot-stub
>> > > > >> features/access-control
>> > > > >> features/locks
>> > > > >> features/worm
>> > > > >> features/read-only
>> > > > >> features/leases
>> > > > >> features/upcall
>> > > > >> performance/io-threads
>> > > > >> features/selinux
>> > > > >> features/marker
>> > > > >> features/barrier
>> > > > >> features/index
>> > > > >> features/quota
>> > > > >> debug/io-stats
>> > > > >> performance/decompounder
>> > > > >> protocol/server
>> > > > >>
>> > > > >>
>> > > > >> For those not following this thread, the question we need to
>> answer
>> > > is,
>> > > > >> "whether the xlator you are associated with works fine if a
>> non-lookup
>> > > > >> fop (like open, setattr, stat etc) hits it without a lookup ever
>> being
>> > > > >> done
>> > > > >> on that inode"
>> > > > >>
>> > > > >> regards,
>> > > > >> Raghavendra
>> > > > >>
>> > > > >> On Wed, Aug 23, 2017 at 11:56 AM, Raghavendra Gowdappa <
>> > > > >> rgowdapp at redhat.com> wrote:
>> > > > >>
>> > > > >>> Thanks Pranith and Ashish for your inputs.
>> > > > >>>
>> > > > >>> ----- Original Message -----
>> > > > >>> > From: "Pranith Kumar Karampuri" <pkarampu at redhat.com>
>> > > > >>> > To: "Ashish Pandey" <aspandey at redhat.com>
>> > > > >>> > Cc: "Raghavendra Gowdappa" <rgowdapp at redhat.com>, "Xavier
>> > > Hernandez" <
>> > > > >>> xhernandez at datalab.es>, "Gluster Devel"
>> > > > >>> > <gluster-devel at gluster.org>
>> > > > >>> > Sent: Wednesday, August 23, 2017 11:55:19 AM
>> > > > >>> > Subject: Re: Need inputs on patch #17985
>> > > > >>> >
>> > > > >>> > Raghavendra,
>> > > > >>> >     As Ashish mentioned, there aren't any known problems if
>> upper
>> > > > >>> xlators
>> > > > >>> > don't send lookups in EC at the moment.
>> > > > >>> >
>> > > > >>> > On Wed, Aug 23, 2017 at 9:07 AM, Ashish Pandey <
>> > > aspandey at redhat.com>
>> > > > >>> wrote:
>> > > > >>> >
>> > > > >>> > > Raghvendra,
>> > > > >>> > >
>> > > > >>> > > I have provided my comment on this patch.
>> > > > >>> > > I think EC will not have any issue with this approach.
>> > > > >>> > > However, I would welcome comments from Xavi and Pranith too
>> for
>> > > any
>> > > > >>> side
>> > > > >>> > > effects which I may not be able to foresee.
>> > > > >>> > >
>> > > > >>> > > Ashish
>> > > > >>> > >
>> > > > >>> > > ------------------------------
>> > > > >>> > > *From: *"Raghavendra Gowdappa" <rgowdapp at redhat.com>
>> > > > >>> > > *To: *"Ashish Pandey" <aspandey at redhat.com>
>> > > > >>> > > *Cc: *"Pranith Kumar Karampuri" <pkarampu at redhat.com>,
>> "Xavier
>> > > > >>> Hernandez"
>> > > > >>> > > <xhernandez at datalab.es>, "Gluster Devel" <
>> > > gluster-devel at gluster.org>
>> > > > >>> > > *Sent: *Wednesday, August 23, 2017 8:29:48 AM
>> > > > >>> > > *Subject: *Need inputs on patch #17985
>> > > > >>> > >
>> > > > >>> > >
>> > > > >>> > > Hi Ashish,
>> > > > >>> > >
>> > > > >>> > > Following are the blockers for making a decision on whether
>> patch
>> > > > >>> [1] can
>> > > > >>> > > be merged or not:
>> > > > >>> > > * Evaluation of dentry operations (like rename etc) in dht
>> > > > >>> > > * Whether EC works fine if a non-lookup fop (like open(dir),
>> > > stat,
>> > > > >>> chmod
>> > > > >>> > > etc) hits EC without a single lookup performed on file/inode
>> > > > >>> > >
>> > > > >>> > > Can you please comment on the patch? I'll take care of dht
>> part.
>> > > > >>> > >
>> > > > >>> > > [1] https://review.gluster.org/#/c/17985/
>> > > > >>> > >
>> > > > >>> > > regards,
>> > > > >>> > > Raghavendra
>> > > > >>> > >
>> > > > >>> > >
>> > > > >>> >
>> > > > >>> >
>> > > > >>> > --
>> > > > >>> > Pranith
>> > > > >>> >
>> > > > >>> _______________________________________________
>> > > > >>> Gluster-devel mailing list
>> > > > >>> Gluster-devel at gluster.org
>> > > > >>> http://lists.gluster.org/mailman/listinfo/gluster-devel
>> > > > >>>
>> > > > >>> --
>> > > > >>> Raghavendra G
>> > > > >>>
>> > > > >>> <http://lists.gluster.org/mailman/listinfo/gluster-devel>
>> > > > >>>
>> > > > >>
>> > > > >> _______________________________________________
>> > > > >> Gluster-devel mailing list
>> > > > >> Gluster-devel at gluster.org
>> > > > >> http://lists.gluster.org/mailman/listinfo/gluster-devel
>> > > > >>
>> > > > >
>> > > > >
>> > > >
>> > > >
>> > > > --
>> > > > Raghavendra G
>> > > >
>> > >
>> >
>>
>
>
>
> --
> Raghavendra G
>

-- 
Raghavendra G
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20171130/1f723f4e/attachment-0001.html>