* QGroups Semantics @ 2017-05-19 9:07 Sargun Dhillon 2017-05-22 0:59 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Sargun Dhillon @ 2017-05-19 9:07 UTC (permalink / raw) To: BTRFS ML I have some questions about the *intended* qgroups semantics, and why we allow certain operations: 1) Why can you create a level 0 qgroup for a non-existent subvolume? It looks like it just gets reset when you create the subvol anyway? Should we prevent this? 2) Why do we allow deleting a level 0 qgroup for a currently existing subvolume? It seems to me that just setting the limit to 0, and/or removing relations would work equally well? Perhaps a new ioctl to clear the qgroup would make sense to do this. 3) Why don't we remove the associated level 0 qgroup when you remove the subvol with that id? 4) I noticed in qgroup.c, one of the outstanding items is to determine how to autocleanup. Are there any stated semantics around that, or opinions? PS: If the answer to any of these is "it just needs to be worked on" -- I'm currently poking around this code path for some enhancements we're doing for our usecase. I'm just trying to figure out how much of what I'm doing is generalizable. -Thanks, Sargun ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QGroups Semantics 2017-05-19 9:07 QGroups Semantics Sargun Dhillon @ 2017-05-22 0:59 ` Qu Wenruo 2017-05-22 20:54 ` Sargun Dhillon 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2017-05-22 0:59 UTC (permalink / raw) To: Sargun Dhillon, BTRFS ML At 05/19/2017 05:07 PM, Sargun Dhillon wrote: > I have some questions about the *intended* qgroups semantics, and why > we allow certain operations: > > 1) Why can you create a level 0 qgroup for a non-existent subvolume? > It looks like it just gets reset when you create the subvol anyway? > Should we prevent this? Personally speaking, I didn't see much meaning of allowing this, except setting limit before creating the subvolume. But that can also be done by setting up higher level qgroup and attach new subvolume to that higher level qgroup. IIRC there are some guys hoping to disable multi-level qgroup completely, if one day we decide to do that, then current behavior may be quite important. > > 2) Why do we allow deleting a level 0 qgroup for a currently existing > subvolume? It seems to me that just setting the limit to 0, and/or > removing relations would work equally well? Perhaps a new ioctl to > clear the qgroup would make sense to do this. At least I didn't see the meaning to allow deleting qgroup for existing subvolume. It won't even improve any performance since we still need to do the time consuming backref walk. > > 3) Why don't we remove the associated level 0 qgroup when you remove > the subvol with that id? This may be related to question 1). Sometimes we may want to keep the limit? Despite of that, I didn't see any reason to keep the orphan level 0 qgroup. > > 4) I noticed in qgroup.c, one of the outstanding items is to determine > how to autocleanup. Are there any stated semantics around that, or > opinions? As long as we're going to stick with multi level qgroups, then autocleanup seems to be a good idea. Thanks, Qu > > PS: > If the answer to any of these is "it just needs to be worked on" -- > I'm currently poking around this code path for some enhancements we're > doing for our usecase. I'm just trying to figure out how much of what > I'm doing is generalizable. > > -Thanks, > Sargun > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QGroups Semantics 2017-05-22 0:59 ` Qu Wenruo @ 2017-05-22 20:54 ` Sargun Dhillon 2017-05-23 1:23 ` Qu Wenruo 2017-05-23 5:38 ` Marat Khalili 0 siblings, 2 replies; 6+ messages in thread From: Sargun Dhillon @ 2017-05-22 20:54 UTC (permalink / raw) To: Qu Wenruo; +Cc: BTRFS ML On Sun, May 21, 2017 at 5:59 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote: > > > At 05/19/2017 05:07 PM, Sargun Dhillon wrote: >> >> I have some questions about the *intended* qgroups semantics, and why >> we allow certain operations: >> >> 1) Why can you create a level 0 qgroup for a non-existent subvolume? >> It looks like it just gets reset when you create the subvol anyway? >> Should we prevent this? > > > Personally speaking, I didn't see much meaning of allowing this, except > setting limit before creating the subvolume. > > But that can also be done by setting up higher level qgroup and attach new > subvolume to that higher level qgroup. > > IIRC there are some guys hoping to disable multi-level qgroup completely, if > one day we decide to do that, then current behavior may be quite important. > >> >> 2) Why do we allow deleting a level 0 qgroup for a currently existing >> subvolume? It seems to me that just setting the limit to 0, and/or >> removing relations would work equally well? Perhaps a new ioctl to >> clear the qgroup would make sense to do this. > > > At least I didn't see the meaning to allow deleting qgroup for existing > subvolume. > > It won't even improve any performance since we still need to do the time > consuming backref walk. > >> >> 3) Why don't we remove the associated level 0 qgroup when you remove >> the subvol with that id? > > This may be related to question 1). > Sometimes we may want to keep the limit? > > Despite of that, I didn't see any reason to keep the orphan level 0 qgroup. > >> >> 4) I noticed in qgroup.c, one of the outstanding items is to determine >> how to autocleanup. Are there any stated semantics around that, or >> opinions? > > > As long as we're going to stick with multi level qgroups, then autocleanup > seems to be a good idea. > > Thanks, > Qu > I propose the following changes: 1) We always cleanup level-0 qgroups by default, with no opt-out. I see absolutely no reason to keep these around. 2) We do not allow the creation of level-0 qgroups for (sub)volumes that do not exist. If people want to have a limit at creation time, they should create a higher level qgroup, and add the new qgroup to that one. 3) We add two new ioctls: qgroup_create_v2, and qgroup_delete_v2, and add a pr_warn_once message when we see usage of the old API. 4) Add a flag to the qgroup_delete_v2 ioctl, NO_SUBVOL_CHECK. If the flag is present, it will allow you to delete qgroups which reference active subvolumes. Opinions? >> >> PS: >> If the answer to any of these is "it just needs to be worked on" -- >> I'm currently poking around this code path for some enhancements we're >> doing for our usecase. I'm just trying to figure out how much of what >> I'm doing is generalizable. >> >> -Thanks, >> Sargun >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QGroups Semantics 2017-05-22 20:54 ` Sargun Dhillon @ 2017-05-23 1:23 ` Qu Wenruo 2017-05-23 5:38 ` Marat Khalili 1 sibling, 0 replies; 6+ messages in thread From: Qu Wenruo @ 2017-05-23 1:23 UTC (permalink / raw) To: Sargun Dhillon; +Cc: BTRFS ML At 05/23/2017 04:54 AM, Sargun Dhillon wrote: > On Sun, May 21, 2017 at 5:59 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote: >> >> >> At 05/19/2017 05:07 PM, Sargun Dhillon wrote: >>> >>> I have some questions about the *intended* qgroups semantics, and why >>> we allow certain operations: >>> >>> 1) Why can you create a level 0 qgroup for a non-existent subvolume? >>> It looks like it just gets reset when you create the subvol anyway? >>> Should we prevent this? >> >> >> Personally speaking, I didn't see much meaning of allowing this, except >> setting limit before creating the subvolume. >> >> But that can also be done by setting up higher level qgroup and attach new >> subvolume to that higher level qgroup. >> >> IIRC there are some guys hoping to disable multi-level qgroup completely, if >> one day we decide to do that, then current behavior may be quite important. >> >>> >>> 2) Why do we allow deleting a level 0 qgroup for a currently existing >>> subvolume? It seems to me that just setting the limit to 0, and/or >>> removing relations would work equally well? Perhaps a new ioctl to >>> clear the qgroup would make sense to do this. >> >> >> At least I didn't see the meaning to allow deleting qgroup for existing >> subvolume. >> >> It won't even improve any performance since we still need to do the time >> consuming backref walk. >> >>> >>> 3) Why don't we remove the associated level 0 qgroup when you remove >>> the subvol with that id? >> >> This may be related to question 1). >> Sometimes we may want to keep the limit? >> >> Despite of that, I didn't see any reason to keep the orphan level 0 qgroup. >> >>> >>> 4) I noticed in qgroup.c, one of the outstanding items is to determine >>> how to autocleanup. Are there any stated semantics around that, or >>> opinions? >> >> >> As long as we're going to stick with multi level qgroups, then autocleanup >> seems to be a good idea. >> >> Thanks, >> Qu >> > I propose the following changes: > 1) We always cleanup level-0 qgroups by default, with no opt-out. > I see absolutely no reason to keep these around. > > 2) We do not allow the creation of level-0 qgroups for (sub)volumes > that do not exist. > If people want to have a limit at creation time, they should create a > higher level qgroup, and add the new qgroup to that one. > > 3) We add two new ioctls: qgroup_create_v2, and qgroup_delete_v2, and > add a pr_warn_once message when we see usage of the old API. > > 4) Add a flag to the qgroup_delete_v2 ioctl, NO_SUBVOL_CHECK. > If the flag is present, it will allow you to delete qgroups which > reference active subvolumes. > > Opinions? I'm completely OK with 3) and 4). While default behavior change in old APIs may cause some old programs to face some unexpected behavior. (That's why I hate to deal with compatibility problems) So I prefer to keep old API as is. Warning message in 3) should be good enough. Thanks, Qu > >>> >>> PS: >>> If the answer to any of these is "it just needs to be worked on" -- >>> I'm currently poking around this code path for some enhancements we're >>> doing for our usecase. I'm just trying to figure out how much of what >>> I'm doing is generalizable. >>> >>> -Thanks, >>> Sargun >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QGroups Semantics 2017-05-22 20:54 ` Sargun Dhillon 2017-05-23 1:23 ` Qu Wenruo @ 2017-05-23 5:38 ` Marat Khalili 2017-05-23 7:38 ` Marat Khalili 1 sibling, 1 reply; 6+ messages in thread From: Marat Khalili @ 2017-05-23 5:38 UTC (permalink / raw) To: BTRFS ML Just some user's point of view: > I propose the following changes: > 1) We always cleanup level-0 qgroups by default, with no opt-out. > I see absolutely no reason to keep these around. It WILL break scripts that try to do this cleanup themselves. OTOH it will simplify writing new ones. Since qgroups are assigned sequential numbers, it must be possible to partially work it around by not returning error on repeated delete. But you cannot completely emulate qgroup presence without actually keeping it, so some scripts will still break. More complete workaround would be delayed cleanup. What about (re-)mount time? (Should also handle qgroups remaining ) > We do not allow the creation of level-0 qgroups for (sub)volumes > that do not exist. Probably I'm mistaken, but I see no reasons for doing it even now, since I don't think it's possible to reliably assign existing 0-level qgroup to a new subvolume. So this change should break nothing. >>> Why do we allow deleting a level 0 qgroup for a currently existing >>> subvolume? > 4) Add a flag to the qgroup_delete_v2 ioctl, NO_SUBVOL_CHECK. > If the flag is present, it will allow you to delete qgroups which > reference active subvolumes. Some people doing cleanup in the reverse order? Other than this, I don't understand why this feature is needed, so IMO it's unlikely to be needed in a new API. Of course, this is all just one datapoint for you. -- With Best Regards, Marat Khalili ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QGroups Semantics 2017-05-23 5:38 ` Marat Khalili @ 2017-05-23 7:38 ` Marat Khalili 0 siblings, 0 replies; 6+ messages in thread From: Marat Khalili @ 2017-05-23 7:38 UTC (permalink / raw) To: BTRFS ML Hit "Send" a little too early: > More complete workaround would be delayed cleanup. What about > (re-)mount time? (Should also handle qgroups remaining ... after subvolumes deleted on previous kernels.) -- With Best Regards, Marat Khalili On 23/05/17 08:38, Marat Khalili wrote: > Just some user's point of view: > >> I propose the following changes: >> 1) We always cleanup level-0 qgroups by default, with no opt-out. >> I see absolutely no reason to keep these around. > > It WILL break scripts that try to do this cleanup themselves. OTOH it > will simplify writing new ones. > > Since qgroups are assigned sequential numbers, it must be possible to > partially work it around by not returning error on repeated delete. > But you cannot completely emulate qgroup presence without actually > keeping it, so some scripts will still break. > > More complete workaround would be delayed cleanup. What about > (re-)mount time? (Should also handle qgroups remaining ) > >> We do not allow the creation of level-0 qgroups for (sub)volumes >> that do not exist. > > Probably I'm mistaken, but I see no reasons for doing it even now, > since I don't think it's possible to reliably assign existing 0-level > qgroup to a new subvolume. So this change should break nothing. > >>>> Why do we allow deleting a level 0 qgroup for a currently existing >>>> subvolume? >> 4) Add a flag to the qgroup_delete_v2 ioctl, NO_SUBVOL_CHECK. >> If the flag is present, it will allow you to delete qgroups which >> reference active subvolumes. > > Some people doing cleanup in the reverse order? Other than this, I > don't understand why this feature is needed, so IMO it's unlikely to > be needed in a new API. > > Of course, this is all just one datapoint for you. > > -- > > With Best Regards, > Marat Khalili > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-23 7:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-19 9:07 QGroups Semantics Sargun Dhillon 2017-05-22 0:59 ` Qu Wenruo 2017-05-22 20:54 ` Sargun Dhillon 2017-05-23 1:23 ` Qu Wenruo 2017-05-23 5:38 ` Marat Khalili 2017-05-23 7:38 ` Marat Khalili
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.