Re: [Gluster-devel] Build on FreeBSD and gcc version
On Wed, Sep 06, 2017 at 12:39:42PM +0200, Michael Scherer wrote: > Hi, > > so I have been trying to make the internal freebsd builder usable, sinc > eit was freshly installed and not building anything. > > Over the course of the day, I found a few missing deps, found a ton of > warnings (I started to fix the easy one, not sure what to do for the > more complicated one around gf_log formating), but yesterday, I > stumbled on a more puzzling issue: > > CCLD glusterfsd > ../../contrib/argp-standalone/libargp.a(argp-help.o): In function > `_help': > /tmp/glusterfs/contrib/argp-standalone/argp-help.c:1608: undefined > reference to `argp_fmtstream_set_wmargin' > /tmp/glusterfs/contrib/argp-standalone/argp-help.c:1622: undefined > reference to `argp_fmtstream_set_lmargin' FreeBSD seems to provide argp-standalone as a port. It would be better to use that instead of our bundled version. https://www.freebsd.org/ports/devel.html#argp-standalone-1.3_2 > I also see message like this: > libtool: warning: relinking 'libgfapi.la' > *** Warning: Linking the shared library libgfapi.la against the > *** static library ../../contrib/argp-standalone/libargp.a is not > portable! libgfapi should not need to link with libargp. This probably is an error in LDFLAGS or something. > After looking at the old builder, I found a few differences: > - old one is FreeBSD 10.1, new one is 10.3 > - old one is using gcc 4.8, new one was on clang. > > So I did install gcc, turn out that this was the version 5 by default > on FreeBSD and it still do not work with it. > > Then I forced gcc4.8 and now it build fine. > Even better, once I forced the version, it also build (but I suspect > that's just some bad cleanup on my side, more tests are needed) > > Any reason on why this would fail on gcc 5, and not 4.8, and what we > should do for that ? I don't know of any reason, but we should fix it somehow. > I also found that what fail is to build only in Jenkins, most likely > due to the setup where we build with a out of source tree directory for > binary. It work fine if doing ./configure ; make ; make install Yeah, this is one of the things that people want to see supported (although I don't remember the reason). It breaks every now and then :-/ HTH, Niels ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [Gluster-Maintainers] Changing Submit Type on review.gluster.org
On Thu, Sep 07, 2017 at 12:17:32PM +0530, Atin Mukherjee wrote: > One basic question (rather clarification) here. If indeed a rebase is > necessary for a patch which was posted some time back and a regression was > passed at that time, with this change will a (centos) regression job > re-triggered and once we have a positive vote then only the patch will be > in to the repo? There's two cases here: * The patch does not have any merge conflicts. In this case, Gerrit will do the rebase when you hit the submit button. There's no question of running regressions. Note: If the patch doesn't have any merge conflicts, you can still do a rebase using Gerrit UI or git command line. This will cause the regression votes to be retained, but the smoke jobs will be retriggered. * The patch has merge conflicts. In this case, you'll have to rebase on the git command line locally and push up a fix. In this case, regressions votes are not carried over and will be triggered. -- nigelb ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [Gluster-Maintainers] Changing Submit Type on review.gluster.org
*Squashed Patches* I believe, individual engineers have to own the responsibility of maintaining history of all appropriate Change-Ids as part of the commit message when multiple patches have been squashed/merged into one commit. On Thu, Sep 7, 2017 at 11:50 AM, Nigel Babu wrote: > Hello folks, > > A few times, we've merged dependent patches out of order because the Submit > type[1] did not block us from doing so. The last few times we've talked > about > this, we didn't actually take a strong decision either way. In yesterday's > maintainers meeting, we agreed to change the Submit type to > Rebase-If-Necessary. This change will happen on 18th September 2017. > > What this means: > * No more metadata flags added by Gerrit. There will only be a Change-Id, > Signed-off-by, and BUG (if you've added it). Gerrit itself will not add > any > metadata. > * If you push a patch on top of another patch, the Submit button will > either be > grayed out because the dependent patches cannot be merged or they will be > submited in the correct order in one go. > > Some of the concerns that have been raised: > Q: With the Reviewed-on flag gone, how do we keep track of changesets >(especially backports)? > A: The Change-Id will get you all the data directly on Gerrit. As long you >retain the Change-Id, Gerrit will get you the matching changesets. > > Q: Will who-wrote-what continue to work? > A: As far as I can see, it continues to work. I ran the script against >build-jobs repo and it works correctly. Additionally, we'll be setting > up an >instance of Gerrit Stats[2] to provide more detailed stats. > > Q: Can we have some of the metadata if not all? > Q: Why can't we have the metadata if we change the submit type? > A: There's no good answer to this other than, this is how Gerrit works and >I can neither change it nor control it. > > [1]: https://review.gluster.org/Documentation/intro-project- > owner.html#submit-type > [2]: http://gerritstats-demo.firebaseapp.com/ > > -- > nigelb > ___ > maintainers mailing list > maintain...@gluster.org > http://lists.gluster.org/mailman/listinfo/maintainers > -- Milind ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] Changing Submit Type on review.gluster.org
On Thu, Sep 07, 2017 at 11:50:21AM +0530, Nigel Babu wrote: > Hello folks, > > A few times, we've merged dependent patches out of order because the Submit > type[1] did not block us from doing so. The last few times we've talked about > this, we didn't actually take a strong decision either way. In yesterday's > maintainers meeting, we agreed to change the Submit type to > Rebase-If-Necessary. This change will happen on 18th September 2017. > > What this means: > * No more metadata flags added by Gerrit. There will only be a Change-Id, > Signed-off-by, and BUG (if you've added it). Gerrit itself will not add any > metadata. > * If you push a patch on top of another patch, the Submit button will either > be > grayed out because the dependent patches cannot be merged or they will be > submited in the correct order in one go. > > Some of the concerns that have been raised: > Q: With the Reviewed-on flag gone, how do we keep track of changesets >(especially backports)? > A: The Change-Id will get you all the data directly on Gerrit. As long you >retain the Change-Id, Gerrit will get you the matching changesets. > > Q: Will who-wrote-what continue to work? > A: As far as I can see, it continues to work. I ran the script against >build-jobs repo and it works correctly. Additionally, we'll be setting up > an >instance of Gerrit Stats[2] to provide more detailed stats. > > Q: Can we have some of the metadata if not all? > Q: Why can't we have the metadata if we change the submit type? > A: There's no good answer to this other than, this is how Gerrit works and >I can neither change it nor control it. Q: Can patches of a series be merged before all patches in the series have a +2? Initial changes that prepare things, or add new (unused) core functionalities should be mergable so that follow-up patches can be posted against the HEAD of the branch. A: Nigel? Thanks, Niels > > [1]: > https://review.gluster.org/Documentation/intro-project-owner.html#submit-type > [2]: http://gerritstats-demo.firebaseapp.com/ > > -- > nigelb > ___ > Gluster-devel mailing list > Gluster-devel@gluster.org > http://lists.gluster.org/mailman/listinfo/gluster-devel ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [Gluster-Maintainers] Changing Submit Type on review.gluster.org
On Thu, Sep 07, 2017 at 12:06:19PM +0530, Amar Tumballi wrote: > On Thu, Sep 7, 2017 at 11:50 AM, Nigel Babu wrote: > > > Hello folks, > > > > A few times, we've merged dependent patches out of order because the Submit > > type[1] did not block us from doing so. The last few times we've talked > > about > > this, we didn't actually take a strong decision either way. In yesterday's > > maintainers meeting, we agreed to change the Submit type to > > Rebase-If-Necessary. This change will happen on 18th September 2017. > > > > What this means: > > * No more metadata flags added by Gerrit. There will only be a Change-Id, > > Signed-off-by, and BUG (if you've added it). Gerrit itself will not add > > any > > metadata. > > * If you push a patch on top of another patch, the Submit button will > > either be > > grayed out because the dependent patches cannot be merged or they will be > > submited in the correct order in one go. > > > > Some of the concerns that have been raised: > > Q: With the Reviewed-on flag gone, how do we keep track of changesets > >(especially backports)? > > A: The Change-Id will get you all the data directly on Gerrit. As long you > >retain the Change-Id, Gerrit will get you the matching changesets. > > > > Q: Will who-wrote-what continue to work? > > A: As far as I can see, it continues to work. I ran the script against > >build-jobs repo and it works correctly. Additionally, we'll be setting > > up an > >instance of Gerrit Stats[2] to provide more detailed stats. > > > > Q: Can we have some of the metadata if not all? > > Q: Why can't we have the metadata if we change the submit type? > > A: There's no good answer to this other than, this is how Gerrit works and > >I can neither change it nor control it. > > > > > One of the major other concerns, which is valid is, stats on 'Reviewed-By'. > Ie, people who actually reviewed the code. This metrics is equally > important as 'who-wrote-the-patch', again, getting an infra like [2] below > will solve it. The /extras/who-wrote-glusterfs/who-wrote-glusterfs.sh script will continue to work, but some of the statistics will not be available. It is nice to have a Reviewed-by tag in the git commit message, but the one that Gerrit adds is not complete in any case. Reviewers that did not review the last version of the patch before it got merged do not get added in the Gerrit-automated-commit-message-ammending. The statistics of the who-wrote-glusterfs.sh script are cool, and people appreciate their names in the git logs, but the script should not be used for serious statistic gathering. The gerritstats webapp seems much more appropriate for this. Cheers, Niels > > Regards, > Amar > > > > [1]: https://review.gluster.org/Documentation/intro-project- > > owner.html#submit-type > > [2]: http://gerritstats-demo.firebaseapp.com/ > > > > -- > > nigelb > > ___ > > maintainers mailing list > > maintain...@gluster.org > > http://lists.gluster.org/mailman/listinfo/maintainers > > > > > > -- > Amar Tumballi (amarts) > ___ > Gluster-devel mailing list > Gluster-devel@gluster.org > http://lists.gluster.org/mailman/listinfo/gluster-devel ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [Gluster-Maintainers] Changing Submit Type on review.gluster.org
On Thu, Sep 07, 2017 at 04:11:22PM +0530, Milind Changire wrote: > *Squashed Patches* > I believe, individual engineers have to own the responsibility of > maintaining history of all appropriate Change-Ids as part of the commit > message when multiple patches have been squashed/merged into one commit. This is accurate. We'll need to continue to maintain this as we currently do for backports. -- nigelb ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] Changing Submit Type on review.gluster.org
On Thu, Sep 07, 2017 at 12:43:28PM +0200, Niels de Vos wrote: > > Q: Can patches of a series be merged before all patches in the series > have a +2? Initial changes that prepare things, or add new (unused) core > functionalities should be mergable so that follow-up patches can be > posted against the HEAD of the branch. > > A: Nigel? > If you have patches that are dependent like this: A -> B -> C -> D where A is the first patch and B is based on top of A and so forth. Merging A is not dependent on B. It can be merged any time you have Code Review and Regression votes. However, you cannot merge B until A is ready or merged. If A is unmerged, but is ready to merge, when you merge B, Gerrit will merge them in order, i.e. first merge A, and B automatically. Does this answer your question? If it helps, I can arrange for staging to be online so moe people can test this out. -- nigelb ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] Build on FreeBSD and gcc version
Le jeudi 07 septembre 2017 à 11:17 +0200, Niels de Vos a écrit : > On Wed, Sep 06, 2017 at 12:39:42PM +0200, Michael Scherer wrote: > > Hi, > > > > so I have been trying to make the internal freebsd builder usable, > > sinc > > eit was freshly installed and not building anything. > > > > Over the course of the day, I found a few missing deps, found a ton > > of > > warnings (I started to fix the easy one, not sure what to do for > > the > > more complicated one around gf_log formating), but yesterday, I > > stumbled on a more puzzling issue: > > > > CCLD glusterfsd > > ../../contrib/argp-standalone/libargp.a(argp-help.o): In function > > `_help': > > /tmp/glusterfs/contrib/argp-standalone/argp-help.c:1608: undefined > > reference to `argp_fmtstream_set_wmargin' > > /tmp/glusterfs/contrib/argp-standalone/argp-help.c:1622: undefined > > reference to `argp_fmtstream_set_lmargin' > > FreeBSD seems to provide argp-standalone as a port. It would be > better > to use that instead of our bundled version. > https://www.freebsd.org/ports/devel.html#argp-standalone-1.3_2 Yeah, I tried, it wasn't picked up. And I am not keen on digging in configure.ac to make it work :) Let's see if my last change on build.sh are fixing it: https://github.com/gluster/glusterfs-patch-acceptance-tests/pull/89 > > > I also see message like this: > > libtool: warning: relinking 'libgfapi.la' > > *** Warning: Linking the shared library libgfapi.la against the > > *** static library ../../contrib/argp-standalone/libargp.a is not > > portable! > > libgfapi should not need to link with libargp. This probably is an > error > in LDFLAGS or something. > > > After looking at the old builder, I found a few differences: > > - old one is FreeBSD 10.1, new one is 10.3 > > - old one is using gcc 4.8, new one was on clang. > > > > So I did install gcc, turn out that this was the version 5 by > > default > > on FreeBSD and it still do not work with it. > > > > Then I forced gcc4.8 and now it build fine. > > Even better, once I forced the version, it also build (but I > > suspect > > that's just some bad cleanup on my side, more tests are needed) > > > > Any reason on why this would fail on gcc 5, and not 4.8, and what > > we > > should do for that ? > > I don't know of any reason, but we should fix it somehow. Yup, but si, it seems it wasn't related to gcc version exactly. But this also bring the question of what should we use on freebsd for testing, since default is clang, and there is 5 versions of gcc. We might also need to clean flags passed to the compiler, since for example -rdynamic is not supported by clang (even if I discovered that flag yesterday) And another question is wether we should push -Werror as well on that platform, since there is a certain amount of warnings that would be hard to fix (most notably the whole issue of pthread_t being a opaque type in POSIX, and that we try to print somehow) both on gcc and clang. But they are technically bugs (and a standard violation too, even if it seems the standard is missing a proper function for that) > > I also found that what fail is to build only in Jenkins, most > > likely > > due to the setup where we build with a out of source tree directory > > for > > binary. It work fine if doing ./configure ; make ; make install > > Yeah, this is one of the things that people want to see supported > (although I don't remember the reason). It breaks every now and then > :-/ It shouldn't, since we test it. But as my 2nd mail show, my debugging lead me to wrong conclusions. > HTH, > Niels -- Michael Scherer Sysadmin, Community Infrastructure and Platform, OSAS signature.asc Description: This is a digitally signed message part ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [Gluster-Maintainers] Changing Submit Type on review.gluster.org
On 09/07/2017 02:20 AM, Nigel Babu wrote: Hello folks, A few times, we've merged dependent patches out of order because the Submit type[1] did not block us from doing so. The last few times we've talked about this, we didn't actually take a strong decision either way. In yesterday's maintainers meeting, we agreed to change the Submit type to Rebase-If-Necessary. This change will happen on 18th September 2017. Unrelated: We possibly need to document on how to work with dependent patches, i.e how to create them? manage intermediate changes, etc. I state this as it is a useful manner of keeping commits small and relevant and easy to review, and building on that practice with better instructions can help. On topic: this would be a welcome change than breaking master when such events happen, or chasing dependencies (determining which patch is dependent on which) when merging the same. What this means: * No more metadata flags added by Gerrit. There will only be a Change-Id, Signed-off-by, and BUG (if you've added it). Gerrit itself will not add any metadata. * If you push a patch on top of another patch, the Submit button will either be grayed out because the dependent patches cannot be merged or they will be submited in the correct order in one go. Some of the concerns that have been raised: Q: With the Reviewed-on flag gone, how do we keep track of changesets (especially backports)? A: The Change-Id will get you all the data directly on Gerrit. As long you retain the Change-Id, Gerrit will get you the matching changesets. Q: Will who-wrote-what continue to work? A: As far as I can see, it continues to work. I ran the script against build-jobs repo and it works correctly. Additionally, we'll be setting up an instance of Gerrit Stats[2] to provide more detailed stats. Q: Can we have some of the metadata if not all? Q: Why can't we have the metadata if we change the submit type? A: There's no good answer to this other than, this is how Gerrit works and I can neither change it nor control it. [1]: https://review.gluster.org/Documentation/intro-project-owner.html#submit-type [2]: http://gerritstats-demo.firebaseapp.com/ -- nigelb ___ maintainers mailing list maintain...@gluster.org http://lists.gluster.org/mailman/listinfo/maintainers ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
[Gluster-devel] Coverity covscan for 2017-09-07-eb2f1ab4 (master branch)
GlusterFS Coverity covscan results are available from http://download.gluster.org/pub/gluster/glusterfs/static-analysis/master/glusterfs-coverity/2017-09-07-eb2f1ab4 ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] Changing Submit Type on review.gluster.org
On Thu, Sep 07, 2017 at 04:41:54PM +0530, Nigel Babu wrote: > On Thu, Sep 07, 2017 at 12:43:28PM +0200, Niels de Vos wrote: > > > > Q: Can patches of a series be merged before all patches in the series > > have a +2? Initial changes that prepare things, or add new (unused) core > > functionalities should be mergable so that follow-up patches can be > > posted against the HEAD of the branch. > > > > A: Nigel? > > > > If you have patches that are dependent like this: > > A -> B -> C -> D > > where A is the first patch and B is based on top of A and so forth. > > Merging A is not dependent on B. It can be merged any time you have Code > Review > and Regression votes. > > However, you cannot merge B until A is ready or merged. If A is unmerged, but > is ready to merge, when you merge B, Gerrit will merge them in order, i.e. > first merge A, and B automatically. > > Does this answer your question? If it helps, I can arrange for staging to be > online so moe people can test this out. That answers my question, I don't need to try it out myself. Thanks! Niels ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [Gluster-Maintainers] Changing Submit Type on review.gluster.org
On 7 Sep 2017 6:25 pm, "Niels de Vos" wrote: On Thu, Sep 07, 2017 at 04:41:54PM +0530, Nigel Babu wrote: > On Thu, Sep 07, 2017 at 12:43:28PM +0200, Niels de Vos wrote: > > > > Q: Can patches of a series be merged before all patches in the series > > have a +2? Initial changes that prepare things, or add new (unused) core > > functionalities should be mergable so that follow-up patches can be > > posted against the HEAD of the branch. > > > > A: Nigel? > > > > If you have patches that are dependent like this: > > A -> B -> C -> D > > where A is the first patch and B is based on top of A and so forth. > > Merging A is not dependent on B. It can be merged any time you have Code Review > and Regression votes. > > However, you cannot merge B until A is ready or merged. If A is unmerged, but > is ready to merge, when you merge B, Gerrit will merge them in order, i.e. > first merge A, and B automatically. > > Does this answer your question? If it helps, I can arrange for staging to be > online so moe people can test this out. That answers my question, I don't need to try it out myself. Thanks! Niels ___ maintainers mailing list maintain...@gluster.org http://lists.gluster.org/mailman/listinfo/maintainers Gerrit still provides all the meta information about patches in a special branch as git-notes. Git can be configured to display these notes along with commit messages. You would still effectively get the same experience as before. More information is available at [1]. This depends on a gerrit plugin, but I believe it's enabled by default. [1] https://gerrit.googlesource.com/plugins/reviewnotes/+/master/src/main/resources/Documentation/refs-notes-review.md ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] [Gluster-Maintainers] Changing Submit Type on review.gluster.org
On Thu, Sep 07, 2017 at 04:11:22PM +0530, Milind Changire wrote: > *Squashed Patches* > I believe, individual engineers have to own the responsibility of > maintaining history of all appropriate Change-Ids as part of the commit > message when multiple patches have been squashed/merged into one commit. We do not recommend squashing multiple patches into one at any case. It is much easier to follow the modifications when each functional change has its own patch. This counts for the master branch, but also for backports. Niels > > > > > On Thu, Sep 7, 2017 at 11:50 AM, Nigel Babu wrote: > > > Hello folks, > > > > A few times, we've merged dependent patches out of order because the Submit > > type[1] did not block us from doing so. The last few times we've talked > > about > > this, we didn't actually take a strong decision either way. In yesterday's > > maintainers meeting, we agreed to change the Submit type to > > Rebase-If-Necessary. This change will happen on 18th September 2017. > > > > What this means: > > * No more metadata flags added by Gerrit. There will only be a Change-Id, > > Signed-off-by, and BUG (if you've added it). Gerrit itself will not add > > any > > metadata. > > * If you push a patch on top of another patch, the Submit button will > > either be > > grayed out because the dependent patches cannot be merged or they will be > > submited in the correct order in one go. > > > > Some of the concerns that have been raised: > > Q: With the Reviewed-on flag gone, how do we keep track of changesets > >(especially backports)? > > A: The Change-Id will get you all the data directly on Gerrit. As long you > >retain the Change-Id, Gerrit will get you the matching changesets. > > > > Q: Will who-wrote-what continue to work? > > A: As far as I can see, it continues to work. I ran the script against > >build-jobs repo and it works correctly. Additionally, we'll be setting > > up an > >instance of Gerrit Stats[2] to provide more detailed stats. > > > > Q: Can we have some of the metadata if not all? > > Q: Why can't we have the metadata if we change the submit type? > > A: There's no good answer to this other than, this is how Gerrit works and > >I can neither change it nor control it. > > > > [1]: https://review.gluster.org/Documentation/intro-project- > > owner.html#submit-type > > [2]: http://gerritstats-demo.firebaseapp.com/ > > > > -- > > nigelb > > ___ > > maintainers mailing list > > maintain...@gluster.org > > http://lists.gluster.org/mailman/listinfo/maintainers > > > > > > -- > Milind > ___ > maintainers mailing list > maintain...@gluster.org > http://lists.gluster.org/mailman/listinfo/maintainers ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
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. >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. Regards, Raghavendra On Tue, Aug 29, 2017 at 12:53 AM, Raghavendra Gowdappa wrote: > > > - Original Message - > > From: "Raghavendra G" > > To: "Nithya Balachandran" > > Cc: "Raghavendra Gowdappa" , anoo...@redhat.com, > "Gluster Devel" , > > raghaven...@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 < > nbala...@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 > > > 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 < > > >> rgowd...@redhat.com> wrote: > > >> > > >>> Thanks Pranith and Ashish for your inputs. > > >>> > > >>> - Original Message - > > >>> > From: "Pranith Kumar Karampuri" > > >>> > To: "Ashish Pandey" > > >>> > Cc: "Raghavendra Gowdappa" , "Xavier > Hernandez" < > > >>> xhernan...@datalab.es>, "Gluster Devel" > > >>> > > > >>> > 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 < > aspan...@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 t
Re: [Gluster-devel] Fuse mounts and inodes
On Wed, Sep 6, 2017 at 4:49 PM, Raghavendra G wrote: > > > On Wed, Sep 6, 2017 at 11:16 AM, Csaba Henk wrote: > >> Thanks Du, nice bit of info! It made me wander about the following: >> >> - Could it be then the default answer we give to "glusterfs client >> high memory usage" >> type of complaints to set vfs_cache_pressure to 100 + x? >> > - And then x = ? Was there proper performance testing done to see how >> performance / >> mem consumtion changes in terms of vfs_cache_performace? >> > > I had a discussion with Manoj on this. One drawback with using > vfs_cache_performance tunable is that its a dynamic algorithm which decides > whether to purge from page cache or inode cache looking at the current > memory pressure. An obvious drawback for glusterfs is that, various of > caches of glusterfs are not visible to kernel (Memory consumed by Glusterfs > gets reflected neither in page cache nor in inode cache). This _might_ > result in algorithm working poorly. > It does not seem to be a big concern. vfs_cache_pressure was brought up in context of the inode proliferation issue, and (unlike "various caches") the size of the gluster client's inode herd is duely reflected in kernel because of the one to one mapping of the inodes of the two (which is the very cause of the phenomenon). Csaba > - vfs_cache_pressure is an allover system tunable. If 100 + x is ideal >> for GlusterFS, can >> we take the courage to propose this? Is there no risk to trash other >> (disk-based) >> filesystems' performace? >> > > That's a valid point. Behavior of other filesystems would be a concern. > > I've not really thought through this suggestion of tuning /proc/sys/vm > tunables and I am not even an expert who knows what tunables are at our > disposal. Just wanted to bring this idea to notice of wider audience. > > >> Csaba >> >> On Wed, Sep 6, 2017 at 6:57 AM, Raghavendra G >> wrote: >> > Another parallel effort could be trying to configure the number of >> > inodes/dentries cached by kernel VFS using /proc/sys/vm interface. >> > >> > == >> > >> > vfs_cache_pressure >> > -- >> > >> > This percentage value controls the tendency of the kernel to reclaim >> > the memory which is used for caching of directory and inode objects. >> > >> > At the default value of vfs_cache_pressure=100 the kernel will attempt >> to >> > reclaim dentries and inodes at a "fair" rate with respect to pagecache >> and >> > swapcache reclaim. Decreasing vfs_cache_pressure causes the kernel to >> > prefer >> > to retain dentry and inode caches. When vfs_cache_pressure=0, the kernel >> > will >> > never reclaim dentries and inodes due to memory pressure and this can >> easily >> > lead to out-of-memory conditions. Increasing vfs_cache_pressure beyond >> 100 >> > causes the kernel to prefer to reclaim dentries and inodes. >> > >> > Increasing vfs_cache_pressure significantly beyond 100 may have negative >> > performance impact. Reclaim code needs to take various locks to find >> > freeable >> > directory and inode objects. With vfs_cache_pressure=1000, it will look >> for >> > ten times more freeable objects than there are. >> > >> > Also we've an article for sysadmins which has a section: >> > >> > >> > >> > With GlusterFS, many users with a lot of storage and many small files >> > easily end up using a lot of RAM on the server side due to >> > 'inode/dentry' caching, leading to decreased performance when the kernel >> > keeps crawling through data-structures on a 40GB RAM system. Changing >> > this value higher than 100 has helped many users to achieve fair caching >> > and more responsiveness from the kernel. >> > >> > >> > >> > Complete article can be found at: >> > https://gluster.readthedocs.io/en/latest/Administrator%20Gui >> de/Linux%20Kernel%20Tuning/ >> > >> > regards, >> > >> > >> > On Tue, Sep 5, 2017 at 5:20 PM, Raghavendra Gowdappa < >> rgowd...@redhat.com> >> > wrote: >> >> >> >> +gluster-devel >> >> >> >> Ashish just spoke to me about need of GC of inodes due to some state in >> >> inode that is being proposed in EC. Hence adding more people to >> >> conversation. >> >> >> >> > > On 4 September 2017 at 12:34, Csaba Henk wrote: >> >> > > >> >> > > > I don't know, depends on how sophisticated GC we need/want/can >> get >> >> > > > by. I >> >> > > > guess the complexity will be inherent, ie. that of the algorithm >> >> > > > chosen >> >> > > > and >> >> > > > how we address concurrency & performance impacts, but once that's >> >> > > > got >> >> > > > right >> >> > > > the other aspects of implementation won't be hard. >> >> > > > >> >> > > > Eg. would it be good just to maintain a simple LRU list? >> >> > > > >> >> > >> >> > Yes. I was also thinking of leveraging lru list. We can invalidate >> first >> >> > "n" >> >> > inodes from lru list of fuse inode table. >> >> > >> >> > > >> >> > > That might work for starters. >> >> > > >> >> > > > >> >> > > > Csa
Re: [Gluster-devel] Need inputs on patch #17985
- Original Message - > From: "FNU Raghavendra Manjunath" > To: "Raghavendra Gowdappa" > Cc: "Raghavendra G" , "Nithya Balachandran" > , anoo...@redhat.com, > "Gluster Devel" , "Raghavendra Bhat" > > 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 > wrote: > > > > > > > - Original Message - > > > From: "Raghavendra G" > > > To: "Nithya Balachandran" > > > Cc: "Raghavendra Gowdappa" , anoo...@redhat.com, > > "Gluster Devel" , > > > raghaven...@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 < > > nbala...@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 > > > > 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
[Gluster-devel] tests/basic/pump.t - what is it used for?
Pranith, I see you're the author of the test in $Subj. Now while I was working on a patch https://review.gluster.org/#/c/18226/ to disallow replace brick operations on dist only volumes the patch failed the regression on this test as the test actually uses replace brick on a distribute only volume which IMO is wrong as then this would always end up in to data loss situation. I'd need some context here to understand the expectation of this test before doing any modifications. ~Atin ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] Metrics: and how to get them out from gluster
Thanks all for the feedback! On Sat, Sep 2, 2017 at 12:21 AM, John Strunk wrote: > > > On Fri, Sep 1, 2017 at 1:27 AM, Amar Tumballi wrote: > >> Disclaimer: This email is long, and did take significant time to write. >> Do take time and read, review and give feedback, so we can have some >> metrics related tasks done by Gluster 4.0 >> >> --- >> ** History:* >> >> To understand what is happening inside GlusterFS process, over the years, >> we have opened many bugs and also coded few things with regard to >> statedump, and did put some effort into io-stats translator to improve the >> gluster's monitoring capabilities. >> >> But surely there is more required! And some glimpse of it is captured in >> [1], [2], [3] & [4]. Also, I did send an email to this group [5] about >> possibilities of capturing this information. >> >> ** Current problem:* >> >> When we talk about metrics or monitoring, we have to consider giving out >> these data to a tool which can preserve the readings in a periodic time, >> without a time graph, no metrics will make sense! So, the first challenge >> itself is how to get them out? Should getting the metrics out from each >> process need 'glusterd' interacting? or should we use signals? Which leads >> us to *'challenge #1'.* >> >> Next is, should we depend on io-stats to do the reporting? If yes, how to >> get information from between any two layers? Should we provide io-stats in >> between all the nodes of translator graph? or should we utilize >> STACK_WIND/UNWIND framework to get the details? This is our *'challenge >> #2'* >> >> Once the above decision will be taken, then the question is, "what about >> 'metrics' from other translators? Who gives it out (ie, dumps it?)? Why do >> we need something similar to statedump, and can't we read info from >> statedump itself?". But when we say 'metrics', we should have a key and a >> number associated with it, statedump has lot more, and no format. If its >> different from statedump, then what is our answer for translator code to >> give out metrics? This is our *'challenge #3*' >> >> If we get a solution to above challenges, then I guess we are in a decent >> shape for further development. Lets go through them one by one, in detail. >> >> ** Problems and proposed solutions:* >> >> *a) how to dump metrics data ?* >> >> Currently, I propose signal handler way, as it will give control for us >> to choose what are the processes we need to capture information on, and >> will be much faster than communicating through another tool. Also >> considering we need to have these metrics taken every 10sec or so, there >> will be a need for efficient way to get this out. >> >> But even there, we have challenges, because we have already chosen both >> USR1 and USR2 signal handlers, one for statedump, another for toggling >> latency monitoring respectively. It makes sense to continue to have >> statedump use USR1, but toggling options should be technically (for >> correctness too) be handled by glusterd volume set options, and there >> should be a way to handle it in a better way by our 'reconfigure()' >> framework in graph-switch. Proposal sent in github issue #303 [6]. >> >> If we are good with above proposal, then we can make use of USR2 for >> metrics dump. Next issue will be about the format of the file itself, which >> we will discuss at the end of the email. >> >> NOTE: Above approach is already implemented in 'experimental' branch, >> excluding handling of [6]. >> > > This was done with SIGUSR2 mainly because for the 'implementation' to test out other things, it was just 1 line change :-) We should surely plan something else IMO too. Will wait for somemore time before doing anything here. > I'm going to pile on with the others discouraging the use of signals and > put a vote in favor of using a network socket. > In a previous project [10], we used a listening TCP socket to provide > metrics to graphite. This has the ability to support multiple receivers by > just sending a copy to each currently open connection, and if there is a > concern about overwhelming receivers and/or slowing down the gluster > sending side, these could be non-blocking sockets that simply drop data if > there is no room in the outbound buffer. The data format we used was > exactly the Graphite text format [11], which includes a timestamp directly > with each metric. The downside is extra data, but it removes > transmission/processing/queuing latency concerns. In practice, we > calculated the timestamp once and used it for all metrics sent in the > interval to minimize the overhead imposed by repeated gettimeofday(). > Another reason I like the socket approach is that in containerized > environments, I can easily run a sidecar that grabs the metrics and > forwards or processes them and it doesn't have to share anything more than > a network port. > > The biggest drawback to the socket approach is it's passive nature. The > receiver is stuck with whatever stat frequency
Re: [Gluster-devel] tests/basic/pump.t - what is it used for?
It was used for testing pump xlator functionality. When replace-brick is done on a distribute volume, it would lead to pump xlator migrating data to the destination brick from source. I guess we can delete this test. I don't think we support pump xlator anymore. On Fri, Sep 8, 2017 at 10:02 AM, Atin Mukherjee wrote: > Pranith, > > I see you're the author of the test in $Subj. Now while I was working on a > patch https://review.gluster.org/#/c/18226/ to disallow replace brick > operations on dist only volumes the patch failed the regression on this > test as the test actually uses replace brick on a distribute only volume > which IMO is wrong as then this would always end up in to data loss > situation. I'd need some context here to understand the expectation of this > test before doing any modifications. > > ~Atin > -- Pranith ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] tests/basic/pump.t - what is it used for?
All I know is pump translator is written long back to support 'replace-brick' without data-loss in distributed setup too. But we figured out pump is not maintainable along the way, and said you can't do a replace-brick in distributed volume type. https://github.com/gluster/glusterfs/commit/acdeed002d30209e0a058c2df0346d 4f16c08994 I don't see that its used now, so looks like we can remove this test. -Amar On Fri, Sep 8, 2017 at 10:02 AM, Atin Mukherjee wrote: > Pranith, > > I see you're the author of the test in $Subj. Now while I was working on a > patch https://review.gluster.org/#/c/18226/ to disallow replace brick > operations on dist only volumes the patch failed the regression on this > test as the test actually uses replace brick on a distribute only volume > which IMO is wrong as then this would always end up in to data loss > situation. I'd need some context here to understand the expectation of this > test before doing any modifications. > > ~Atin > > ___ > Gluster-devel mailing list > Gluster-devel@gluster.org > http://lists.gluster.org/mailman/listinfo/gluster-devel > -- Amar Tumballi (amarts) ___ Gluster-devel mailing list Gluster-devel@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-devel