GNU bug report logs - #17470
[PATCH] sort: rotate on ENOSPC while creating tmp files

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17470 in the body.
You can then email your comments to 17470 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Azat Khuzhin <a3at.mail <at> gmail.com>
To: bug-coreutils <at> gnu.org
Cc: Azat Khuzhin <a3at.mail <at> gmail.com>
Subject: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Mon, 12 May 2014 00:37:15 +0400

This can be useful in case you use partitions with different free space
on them. It will better to go to the next partition if we don't have
space on current one, instead of fail.

* src/sort.c (create_temp_file): Go through all available tmp dirs if we
got ENOSPC for the first one, and fail on the last.
---
Here is the RFC, comments are welcome. Thanks.

 src/sort.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index 3380be6..47348b7 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -853,22 +853,36 @@ create_temp_file (int *pfd, bool survive_fd_exhaustion)
   static size_t temp_dir_index;
   int fd;
   int saved_errno;
-  char const *temp_dir = temp_dirs[temp_dir_index];
-  size_t len = strlen (temp_dir);
-  struct tempnode *node =
-    xmalloc (offsetof (struct tempnode, name) + len + sizeof slashbase);
-  char *file = node->name;
+  char const *temp_dir;
+  size_t len;
+  struct tempnode *node = NULL;
+  char *file;
   struct cs_status cs;
-
-  memcpy (file, temp_dir, len);
-  memcpy (file + len, slashbase, sizeof slashbase);
-  node->next = NULL;
-  if (++temp_dir_index == temp_dir_count)
-    temp_dir_index = 0;
+  size_t start_dir_index = temp_dir_index;
 
   /* Create the temporary file in a critical section, to avoid races.  */
   cs = cs_enter ();
-  fd = mkstemp (file);
+  do
+    {
+      temp_dir = temp_dirs[temp_dir_index];
+      len = strlen (temp_dir);
+      node =
+        xrealloc (node,
+                  offsetof (struct tempnode, name) + len + sizeof slashbase);
+      file = node->name;
+      memcpy (file, temp_dir, len);
+      memcpy (file + len, slashbase, sizeof slashbase);
+      node->next = NULL;
+
+      if (++temp_dir_index == temp_dir_count)
+        temp_dir_index = 0;
+
+      fd = mkstemp (file);
+
+      if (errno != ENOSPC || temp_dir_index == start_dir_index)
+        break;
+    } while (0 <= fd);
+
   if (0 <= fd)
     {
       *temptail = node;
-- 
2.0.0.rc0

Message #8 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Azat Khuzhin <a3at.mail <at> gmail.com>, 17470 <at> debbugs.gnu.org
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Sun, 11 May 2014 15:25:56 -0700

Azat Khuzhin wrote:

> +      fd = mkstemp (file);
> +
> +      if (errno != ENOSPC || temp_dir_index == start_dir_index)

This assumes that when mkstemp succeeds then errno != ENOSPC, which is 
not necessarily true.

More generally, it appears that with the patch 'sort' checks whether one 
can create a file, but 'sort' will still respond poorly if a write to a 
temp file fails due to filesystem space exhaustion.

Message #11 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Azat Khuzhin <a3at.mail <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 17470 <at> debbugs.gnu.org
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Mon, 12 May 2014 02:44:51 +0400

[Message part 1 (text/plain, inline)]

On 12 May 2014 02:26, "Paul Eggert" <eggert <at> cs.ucla.edu> wrote:
>
> Azat Khuzhin wrote:
>
>> +      fd = mkstemp (file);
>> +
>> +      if (errno != ENOSPC || temp_dir_index == start_dir_index)
>
>
> This assumes that when mkstemp succeeds then errno != ENOSPC, which is
not necessarily true.

Why that could be, only if there will be an old value?
End even if it is true it will not go to the next iteration because fd >= 0

>
> More generally, it appears that with the patch 'sort' checks whether one
can create a file, but 'sort' will still respond poorly if a write to a
temp file fails due to filesystem space exhaustion.

The only thing that will slow down 'sort' is going through tmp dirs where
there is no enough space, and I personally don't think that this will
meaningful, since most generic use case to use tmp dirs is to sort data
that will not fit into memory and if this sort will fail because of enospc
you must to restart the sort, which can be running for day or weak already,
and this is more expensive.
Or I misunderstood something?

And now I realize that this is not enough, since we only checking on
creation, and unfortunately I didn't check how write(2) handle errors, if
it try to create another file than it will work.
We also could use fallocate here.

Thanks.
Azat.

[Message part 2 (text/html, inline)]

Message #14 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Azat Khuzhin <a3at.mail <at> gmail.com>
Cc: 17470 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Mon, 12 May 2014 00:22:45 +0100

tag 17470 wontfix
close 17470
stop

On 05/11/2014 11:25 PM, Paul Eggert wrote:
> Azat Khuzhin wrote:
> 
>> +      fd = mkstemp (file);
>> +
>> +      if (errno != ENOSPC || temp_dir_index == start_dir_index)
> 
> This assumes that when mkstemp succeeds then errno != ENOSPC, which is not necessarily true.
> 
> More generally, it appears that with the patch 'sort' checks whether one can create a file, but 'sort' will still respond poorly if a write to a temp file fails due to filesystem space exhaustion.

Yes I agree.

Now one could use fallocate() where available to preallocate a given amount of space,
however allocation management can be done outside of sort(1). As a rule of thumb,
if it's possible to implement outside of a particular functional unit, then it
probably should be done outside.

In this case there are various schemes for coalescing multiple storage locations
to a single mount point (mhddfs, lvm, raid, ...), and since these have a more
system wide view, it would be better to avoid implementing similar but limited
logic within sort.

thanks,
Pádraig.

Message #21 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Azat Khuzhin <a3at.mail <at> gmail.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 17470 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Wed, 14 May 2014 15:07:02 +0400

On Mon, May 12, 2014 at 12:22:45AM +0100, Pádraig Brady wrote:
> tag 17470 wontfix
> close 17470
> stop
> 
> On 05/11/2014 11:25 PM, Paul Eggert wrote:
> > Azat Khuzhin wrote:
> > 
> >> +      fd = mkstemp (file);
> >> +
> >> +      if (errno != ENOSPC || temp_dir_index == start_dir_index)
> > 
> > This assumes that when mkstemp succeeds then errno != ENOSPC, which is not necessarily true.
> > 
> > More generally, it appears that with the patch 'sort' checks whether one can create a file, but 'sort' will still respond poorly if a write to a temp file fails due to filesystem space exhaustion.
> 
> Yes I agree.
> 
> Now one could use fallocate() where available to preallocate a given amount of space,
> however allocation management can be done outside of sort(1). As a rule of thumb,
> if it's possible to implement outside of a particular functional unit, then it
> probably should be done outside.

Sometimes it's not possible to do this, because it will likely need in
erasing data in all involved partitions, or it can make _all_ data
unavailable when one of disks will fail.
And it more simpler to use just sort(1) instead of
fdisk/pvcreate/mdadm/...(1).
Occasionally, even restart can be painfull.

And this patch is relatively small, so this is not even an _allocation
managemenet_.
Maybe if I will update patch to do this only under specific option, what
you think?

Thanks,
Azat.

> 
> In this case there are various schemes for coalescing multiple storage locations
> to a single mount point (mhddfs, lvm, raid, ...), and since these have a more
> system wide view, it would be better to avoid implementing similar but limited
> logic within sort.
> 
> thanks,
> Pádraig.

Message #24 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Azat Khuzhin <a3at.mail <at> gmail.com>
Cc: 17470 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Wed, 14 May 2014 12:24:46 +0100

On 05/14/2014 12:07 PM, Azat Khuzhin wrote:
> On Mon, May 12, 2014 at 12:22:45AM +0100, Pádraig Brady wrote:
>> tag 17470 wontfix
>> close 17470
>> stop
>>
>> On 05/11/2014 11:25 PM, Paul Eggert wrote:
>>> Azat Khuzhin wrote:
>>>
>>>> +      fd = mkstemp (file);
>>>> +
>>>> +      if (errno != ENOSPC || temp_dir_index == start_dir_index)
>>>
>>> This assumes that when mkstemp succeeds then errno != ENOSPC, which is not necessarily true.
>>>
>>> More generally, it appears that with the patch 'sort' checks whether one can create a file, but 'sort' will still respond poorly if a write to a temp file fails due to filesystem space exhaustion.
>>
>> Yes I agree.
>>
>> Now one could use fallocate() where available to preallocate a given amount of space,
>> however allocation management can be done outside of sort(1). As a rule of thumb,
>> if it's possible to implement outside of a particular functional unit, then it
>> probably should be done outside.
> 
> Sometimes it's not possible to do this, because it will likely need in
> erasing data in all involved partitions, or it can make _all_ data
> unavailable when one of disks will fail.

There are many external solutions to that problem.

> And it more simpler to use just sort(1) instead of
> fdisk/pvcreate/mdadm/...(1).
> Occasionally, even restart can be painfull.
> 
> And this patch is relatively small, so this is not even an _allocation
> managemenet_.
> Maybe if I will update patch to do this only under specific option, what
> you think?

Well it shouldn't need any interface changes as sort already accepts multiple -T options.
However it wouldn't be that simple either requiring fallocations, which are not
generally available and which don't guarantee that writes will not give ENOSPC.
Also do we always know how much to fallocate? Would that have efficiency problems
compared to dynamic allocate? What about sort --compress, ...

So still not convinced sorry.

Pádraig.

Message #27 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Azat Khuzhin <a3at.mail <at> gmail.com>
Cc: 17470 <at> debbugs.gnu.org
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Wed, 14 May 2014 07:48:39 -0700

Pádraig Brady wrote:
> Also do we always know how much to fallocate?

Not if we're using compression on the temporaries, no.

I think a patch along these lines could be worthwhile, if it was simple 
and if it actually worked (the current one doesn't).  Something along 
the following lines, say.  When multiple -T options are specified (-T 
FOO, -T FOP, -T FOQ, ...) and one of them runs out of disk space when 
creating a temporary file FOO/BAR, 'sort' stops creating files in FOO 
(effectively removing FOO from the option list) creates a file FOP/BAR 
instead, and redoes the process (whatever it was) that sent output to 
FOO/BAR, sending the output to FOP/BAR this time.

I don't have the energy right now to write that, but if someone else 
wrote it I'd review it.

Message #30 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Azat Khuzhin <a3at.mail <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 17470 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Mon, 26 May 2014 23:44:29 +0400

On Wed, May 14, 2014 at 07:48:39AM -0700, Paul Eggert wrote:
> Pádraig Brady wrote:
> >Also do we always know how much to fallocate?
> 
> Not if we're using compression on the temporaries, no.
> 
> I think a patch along these lines could be worthwhile, if it was simple and
> if it actually worked (the current one doesn't).  

The current patch only look while files is created, but this is not
enough `I agree with you, it must check write(2) and fallback to creating
when write(2) will fail with ENOSPC.
This is what you mean?

> Something along the
> following lines, say.  When multiple -T options are specified (-T FOO, -T
> FOP, -T FOQ, ...) and one of them runs out of disk space when creating a
> temporary file FOO/BAR, 'sort' stops creating files in FOO (effectively
> removing FOO from the option list) creates a file FOP/BAR instead, and
> redoes the process (whatever it was) that sent output to FOO/BAR, sending
> the output to FOP/BAR this time.

I don't think that redoes is worth it, since when we have ENOSPC it
means that we already won't create any more files there, and one file
with relatively small size is not a big deal.
I also think that dropping direcotory from list is a good idea, user can
notice this (using some monitorings) and clean it.

For example recently I need to sort relatively huge amount of data, and
I don't have enough space for writing all tmp files (why
--compress-program not works for me is another story), so I wrote
script, that did next:
When free space < %3 on some of partitions, I run pkill -STOP sort, and
then move some files (that not currently opened by sort(1)) to partition
that have more free space, and create symlinks for old locations.
When all the free space was eliminated on all available partitions, I
archived existed tmp files (that also not opened by sort(1)) and create
a pipes, that redirects output of $(gzip -d) into it, and using this
hacks sort finished successfully for me.


> 
> I don't have the energy right now to write that, but if someone else wrote
> it I'd review it.

Thanks for you notes Pádraig.

-- 
Respectfully
Azat Khuzhin

Message #33 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Azat Khuzhin <a3at.mail <at> gmail.com>
Cc: 17470 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Mon, 26 May 2014 13:44:14 -0700

Azat Khuzhin wrote:
> The current patch only look while files is created, but this is not
> enough `I agree with you, it must check write(2) and fallback to creating
> when write(2) will fail with ENOSPC.
> This is what you mean?

Yes.

> when we have ENOSPC it
> means that we already won't create any more files there, and one file
> with relatively small size is not a big deal.

OK.  The point is that 'sort' shouldn't lose the data (including the 
possibly-incomplete trailing line) that's already in the temporary file 
when a write to that file fails.

Also, the code could treat EIO like ENOSPC, I suppose, to be more robust 
in the presence of bad temporary devices.

But beware file systems that report ENOSPC and EIO in a delayed fashion, 
i.e., not immediately upon the failing write, but somewhat later, 
typically when closing the output file.

Message #36 received at 17470 <at> debbugs.gnu.org (full text, mbox):

From: Azat Khuzhin <a3at.mail <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 17470 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#17470: [PATCH] sort: rotate on ENOSPC while creating tmp files
Date: Tue, 27 May 2014 00:56:11 +0400

On Mon, May 26, 2014 at 01:44:14PM -0700, Paul Eggert wrote:
> Azat Khuzhin wrote:
> >The current patch only look while files is created, but this is not
> >enough `I agree with you, it must check write(2) and fallback to creating
> >when write(2) will fail with ENOSPC.
> >This is what you mean?
> 
> Yes.
> 
> >when we have ENOSPC it
> >means that we already won't create any more files there, and one file
> >with relatively small size is not a big deal.
> 
> OK.  The point is that 'sort' shouldn't lose the data (including the
> possibly-incomplete trailing line) that's already in the temporary file when
> a write to that file fails.

Thanks for the explanation, I see what you mean.

> 
> Also, the code could treat EIO like ENOSPC, I suppose, to be more robust in
> the presence of bad temporary devices.
> 
> But beware file systems that report ENOSPC and EIO in a delayed fashion,
> i.e., not immediately upon the failing write, but somewhat later, typically
> when closing the output file.

Yeah, that's a good catch, I will keep this in mind when I will start
working on this.

Thanks,
Azat.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.