Re: [RFC PATCH 02/02] swapon: add "cluster-discard" support
KOSAKI Motohiro writes: >> Instead of reverting and renaming --discard, what about making it accept an >> optional argument, so we could use --discard (to enable all thing and keep >> backward compatibility); --discard=cluster & --discard=batch (or whatever we >> think it should be named). I'll try to sort this approach out if you folks >> think >> it's worthwhile. > > Optional argument looks nice, at least to me. > > But hmm.. > > "cluster" and "batch" describes current kernel implementation, not user > visible effect. > Usually I suggest to pick up a word from man pages because it describe user > visible action. > e.g. --discard=freed-pages or --discard=io or --discard=swapon or > --discard=once, etc.. > > But this is not strong opinion. You can ignore it. I don't think I have good > English sense. :-) I like discard=swapon. For the fine-grained discards, I don't have a strong opinion, but I guess I'd lean towards freed-pages. Cheers, Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add cluster-discard support
KOSAKI Motohiro kosaki.motoh...@gmail.com writes: Instead of reverting and renaming --discard, what about making it accept an optional argument, so we could use --discard (to enable all thing and keep backward compatibility); --discard=cluster --discard=batch (or whatever we think it should be named). I'll try to sort this approach out if you folks think it's worthwhile. Optional argument looks nice, at least to me. But hmm.. cluster and batch describes current kernel implementation, not user visible effect. Usually I suggest to pick up a word from man pages because it describe user visible action. e.g. --discard=freed-pages or --discard=io or --discard=swapon or --discard=once, etc.. But this is not strong opinion. You can ignore it. I don't think I have good English sense. :-) I like discard=swapon. For the fine-grained discards, I don't have a strong opinion, but I guess I'd lean towards freed-pages. Cheers, Jeff -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add "cluster-discard" support
> Instead of reverting and renaming --discard, what about making it accept an > optional argument, so we could use --discard (to enable all thing and keep > backward compatibility); --discard=cluster & --discard=batch (or whatever we > think it should be named). I'll try to sort this approach out if you folks > think > it's worthwhile. Optional argument looks nice, at least to me. But hmm.. "cluster" and "batch" describes current kernel implementation, not user visible effect. Usually I suggest to pick up a word from man pages because it describe user visible action. e.g. --discard=freed-pages or --discard=io or --discard=swapon or --discard=once, etc.. But this is not strong opinion. You can ignore it. I don't think I have good English sense. :-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add "cluster-discard" support
Karel, Motohiro,
Thanks a lot for your time reviewing this patch and providing me with valuable
feedback.
On Tue, May 21, 2013 at 04:17:04PM -0400, KOSAKI Motohiro wrote:
> (5/21/13 6:26 AM), Karel Zak wrote:
> > On Mon, May 20, 2013 at 09:02:43PM -0400, KOSAKI Motohiro wrote:
> >>> - if (fl_discard)
> >>> + if (fl_discard) {
> >>> flags |= SWAP_FLAG_DISCARD;
> >>> + if (fl_discard > 1)
> >>> + flags |= SWAP_FLAG_DISCARD_CLUSTER;
> >>
> >> This is not enough, IMHO. When running this code on old kernel, swapon()
> >> return EINVAL.
> >> At that time, we should fall back swapon(0x1).
> >
> > Hmm.. currently we don't use any fallback for any swap flag (e.g.
> > 0x1) for compatibility with old kernels. Maybe it's better to
> > keep it simple and stupid and return an error message than introduce
> > any super-smart semantic to hide incompatible fstab configuration.
>
> Hm. If so, I'd propose to revert the following change.
>
> > .B "\-d, \-\-discard"
> >-Discard freed swap pages before they are reused, if the swap
> >-device supports the discard or trim operation. This may improve
> >-performance on some Solid State Devices, but often it does not.
> >+Enables swap discards, if the swap device supports that, and performs
> >+a batch discard operation for the swap device at swapon time.
>
>
> And instead, I suggest to make --discard-on-swapon like the following.
> (better name idea is welcome)
>
> +--discard-on-swapon
> +Enables swap discards, if the swap device supports that, and performs
> +a batch discard operation for the swap device at swapon time.
>
> I mean, preserving flags semantics removes the reason we need make a fallback.
>
>
Instead of reverting and renaming --discard, what about making it accept an
optional argument, so we could use --discard (to enable all thing and keep
backward compatibility); --discard=cluster & --discard=batch (or whatever we
think it should be named). I'll try to sort this approach out if you folks think
it's worthwhile.
-- Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add "cluster-discard" support
(5/21/13 6:26 AM), Karel Zak wrote:
> On Mon, May 20, 2013 at 09:02:43PM -0400, KOSAKI Motohiro wrote:
>>> - if (fl_discard)
>>> + if (fl_discard) {
>>> flags |= SWAP_FLAG_DISCARD;
>>> + if (fl_discard > 1)
>>> + flags |= SWAP_FLAG_DISCARD_CLUSTER;
>>
>> This is not enough, IMHO. When running this code on old kernel, swapon()
>> return EINVAL.
>> At that time, we should fall back swapon(0x1).
>
> Hmm.. currently we don't use any fallback for any swap flag (e.g.
> 0x1) for compatibility with old kernels. Maybe it's better to
> keep it simple and stupid and return an error message than introduce
> any super-smart semantic to hide incompatible fstab configuration.
Hm. If so, I'd propose to revert the following change.
> .B "\-d, \-\-discard"
>-Discard freed swap pages before they are reused, if the swap
>-device supports the discard or trim operation. This may improve
>-performance on some Solid State Devices, but often it does not.
>+Enables swap discards, if the swap device supports that, and performs
>+a batch discard operation for the swap device at swapon time.
And instead, I suggest to make --discard-on-swapon like the following.
(better name idea is welcome)
+--discard-on-swapon
+Enables swap discards, if the swap device supports that, and performs
+a batch discard operation for the swap device at swapon time.
I mean, preserving flags semantics removes the reason we need make a fallback.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add "cluster-discard" support
On Mon, May 20, 2013 at 09:02:43PM -0400, KOSAKI Motohiro wrote:
> > - if (fl_discard)
> > + if (fl_discard) {
> > flags |= SWAP_FLAG_DISCARD;
> > + if (fl_discard > 1)
> > + flags |= SWAP_FLAG_DISCARD_CLUSTER;
>
> This is not enough, IMHO. When running this code on old kernel, swapon()
> return EINVAL.
> At that time, we should fall back swapon(0x1).
Hmm.. currently we don't use any fallback for any swap flag (e.g.
0x1) for compatibility with old kernels. Maybe it's better to
keep it simple and stupid and return an error message than introduce
any super-smart semantic to hide incompatible fstab configuration.
Karel
--
Karel Zak
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add "cluster-discard" support
On Mon, May 20, 2013 at 09:04:25PM -0300, Rafael Aquini wrote:
> - while ((c = getopt_long(argc, argv, "ahdefp:svVL:U:",
> + while ((c = getopt_long(argc, argv, "ahcdefp:svVL:U:",
> long_opts, NULL)) != -1) {
> switch (c) {
> case 'a': /* all */
> @@ -738,8 +753,11 @@ int main(int argc, char *argv[])
> case 'U':
> add_uuid(optarg);
> break;
> + case 'c':
> + discard += 2;
> + break;
> case 'd':
> - discard = 1;
> + discard += 1;
this is fragile, it would be better to use
case 'c':
discard |= SWAP_FLAG_DISCARD_CLUSTER;
break;
case 'd':
discard |= SWAP_FLAG_DISCARD;
break;
and use directly the flags everywhere in the code than use magical
numbers '1' and '2' etc.
Karel
--
Karel Zak
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add cluster-discard support
On Mon, May 20, 2013 at 09:04:25PM -0300, Rafael Aquini wrote:
- while ((c = getopt_long(argc, argv, ahdefp:svVL:U:,
+ while ((c = getopt_long(argc, argv, ahcdefp:svVL:U:,
long_opts, NULL)) != -1) {
switch (c) {
case 'a': /* all */
@@ -738,8 +753,11 @@ int main(int argc, char *argv[])
case 'U':
add_uuid(optarg);
break;
+ case 'c':
+ discard += 2;
+ break;
case 'd':
- discard = 1;
+ discard += 1;
this is fragile, it would be better to use
case 'c':
discard |= SWAP_FLAG_DISCARD_CLUSTER;
break;
case 'd':
discard |= SWAP_FLAG_DISCARD;
break;
and use directly the flags everywhere in the code than use magical
numbers '1' and '2' etc.
Karel
--
Karel Zak k...@redhat.com
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add cluster-discard support
On Mon, May 20, 2013 at 09:02:43PM -0400, KOSAKI Motohiro wrote:
- if (fl_discard)
+ if (fl_discard) {
flags |= SWAP_FLAG_DISCARD;
+ if (fl_discard 1)
+ flags |= SWAP_FLAG_DISCARD_CLUSTER;
This is not enough, IMHO. When running this code on old kernel, swapon()
return EINVAL.
At that time, we should fall back swapon(0x1).
Hmm.. currently we don't use any fallback for any swap flag (e.g.
0x1) for compatibility with old kernels. Maybe it's better to
keep it simple and stupid and return an error message than introduce
any super-smart semantic to hide incompatible fstab configuration.
Karel
--
Karel Zak k...@redhat.com
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add cluster-discard support
(5/21/13 6:26 AM), Karel Zak wrote:
On Mon, May 20, 2013 at 09:02:43PM -0400, KOSAKI Motohiro wrote:
- if (fl_discard)
+ if (fl_discard) {
flags |= SWAP_FLAG_DISCARD;
+ if (fl_discard 1)
+ flags |= SWAP_FLAG_DISCARD_CLUSTER;
This is not enough, IMHO. When running this code on old kernel, swapon()
return EINVAL.
At that time, we should fall back swapon(0x1).
Hmm.. currently we don't use any fallback for any swap flag (e.g.
0x1) for compatibility with old kernels. Maybe it's better to
keep it simple and stupid and return an error message than introduce
any super-smart semantic to hide incompatible fstab configuration.
Hm. If so, I'd propose to revert the following change.
.B \-d, \-\-discard
-Discard freed swap pages before they are reused, if the swap
-device supports the discard or trim operation. This may improve
-performance on some Solid State Devices, but often it does not.
+Enables swap discards, if the swap device supports that, and performs
+a batch discard operation for the swap device at swapon time.
And instead, I suggest to make --discard-on-swapon like the following.
(better name idea is welcome)
+--discard-on-swapon
+Enables swap discards, if the swap device supports that, and performs
+a batch discard operation for the swap device at swapon time.
I mean, preserving flags semantics removes the reason we need make a fallback.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add cluster-discard support
Karel, Motohiro,
Thanks a lot for your time reviewing this patch and providing me with valuable
feedback.
On Tue, May 21, 2013 at 04:17:04PM -0400, KOSAKI Motohiro wrote:
(5/21/13 6:26 AM), Karel Zak wrote:
On Mon, May 20, 2013 at 09:02:43PM -0400, KOSAKI Motohiro wrote:
- if (fl_discard)
+ if (fl_discard) {
flags |= SWAP_FLAG_DISCARD;
+ if (fl_discard 1)
+ flags |= SWAP_FLAG_DISCARD_CLUSTER;
This is not enough, IMHO. When running this code on old kernel, swapon()
return EINVAL.
At that time, we should fall back swapon(0x1).
Hmm.. currently we don't use any fallback for any swap flag (e.g.
0x1) for compatibility with old kernels. Maybe it's better to
keep it simple and stupid and return an error message than introduce
any super-smart semantic to hide incompatible fstab configuration.
Hm. If so, I'd propose to revert the following change.
.B \-d, \-\-discard
-Discard freed swap pages before they are reused, if the swap
-device supports the discard or trim operation. This may improve
-performance on some Solid State Devices, but often it does not.
+Enables swap discards, if the swap device supports that, and performs
+a batch discard operation for the swap device at swapon time.
And instead, I suggest to make --discard-on-swapon like the following.
(better name idea is welcome)
+--discard-on-swapon
+Enables swap discards, if the swap device supports that, and performs
+a batch discard operation for the swap device at swapon time.
I mean, preserving flags semantics removes the reason we need make a fallback.
Instead of reverting and renaming --discard, what about making it accept an
optional argument, so we could use --discard (to enable all thing and keep
backward compatibility); --discard=cluster --discard=batch (or whatever we
think it should be named). I'll try to sort this approach out if you folks think
it's worthwhile.
-- Rafael
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add cluster-discard support
Instead of reverting and renaming --discard, what about making it accept an optional argument, so we could use --discard (to enable all thing and keep backward compatibility); --discard=cluster --discard=batch (or whatever we think it should be named). I'll try to sort this approach out if you folks think it's worthwhile. Optional argument looks nice, at least to me. But hmm.. cluster and batch describes current kernel implementation, not user visible effect. Usually I suggest to pick up a word from man pages because it describe user visible action. e.g. --discard=freed-pages or --discard=io or --discard=swapon or --discard=once, etc.. But this is not strong opinion. You can ignore it. I don't think I have good English sense. :-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add "cluster-discard" support
> +.B "\-c, \-\-cluster\-discard"
> +Swapping will discard clusters of swap pages in between freeing them
> +and re-writing to them, if the swap device supports that. This option
> +also implies the
> +.I \-d, \-\-discard
> +swapon flag.
I'm not sure this is good idea. Why can't we make these flags orthogonal?
> /* If true, don't complain if the device/file doesn't exist */
> static int ifexists;
> @@ -570,8 +574,11 @@ static int do_swapon(const char *orig_special, int prio,
> << SWAP_FLAG_PRIO_SHIFT);
> }
> #endif
> - if (fl_discard)
> + if (fl_discard) {
> flags |= SWAP_FLAG_DISCARD;
> + if (fl_discard > 1)
> + flags |= SWAP_FLAG_DISCARD_CLUSTER;
This is not enough, IMHO. When running this code on old kernel, swapon() return
EINVAL.
At that time, we should fall back swapon(0x1).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/02] swapon: add cluster-discard support
+.B \-c, \-\-cluster\-discard
+Swapping will discard clusters of swap pages in between freeing them
+and re-writing to them, if the swap device supports that. This option
+also implies the
+.I \-d, \-\-discard
+swapon flag.
I'm not sure this is good idea. Why can't we make these flags orthogonal?
/* If true, don't complain if the device/file doesn't exist */
static int ifexists;
@@ -570,8 +574,11 @@ static int do_swapon(const char *orig_special, int prio,
SWAP_FLAG_PRIO_SHIFT);
}
#endif
- if (fl_discard)
+ if (fl_discard) {
flags |= SWAP_FLAG_DISCARD;
+ if (fl_discard 1)
+ flags |= SWAP_FLAG_DISCARD_CLUSTER;
This is not enough, IMHO. When running this code on old kernel, swapon() return
EINVAL.
At that time, we should fall back swapon(0x1).
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
