bugGNU GRUB - Bugs: bug #32472, Shouldn't use sync ()

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #32472: Shouldn't use sync ()

Submitter:  Коренберг Марк <socketpair>
Submitted:  Mon 14 Feb 2011 09:31:27 AM UTC
   
 
Category:  Installation Severity:  Major
Priority:  5 - Normal Item Group:  Software Error
Status:  Fixed Privacy:  Public
Assigned to:  None Originator Name:  Коренберг Марк
Open/Closed:  Closed Release:  1.98
Release:  Reproducibility:  Every Time
Planned Release:  None

Jump to the original submission

Mon 25 Apr 2011 05:40:30 PM UTC, comment #19: 

You don't have partitions.

Vladimir Serbinenko <phcoder>
Group administrator
Mon 25 Apr 2011 04:12:38 PM UTC, comment #18: 

What I was checked:

# truncate -s 100M qwe
# mkdir qwed
# mkfs.ext4 qwe
# mount -o loop qwe qwed
# cp -a /boot/grub qwed/
# echo '(hd0) qwe' > qwe.map
# ./grub-setup  -m qwe.map qwe -r '(hd0)' -s -f -d qwed/grub
# umount -f qwed

in both cases (patched and unpatched) operation is successfull.

What I do wrong ?

Коренберг Марк <socketpair>
Mon 25 Apr 2011 12:09:41 PM UTC, comment #17: 


>> Calling grub_util_biosdisk_flush in grub-setup makes the difference between failed and successfull install



>What did you mean ?
>grub_util_biosdisk_flush() is completely "read-only" i.e. does not change anything, return code does not checked anywhere.

You can try it yourself. Comment all the calls to grub_util_biosdisk_flush and try to install in blocklist mode (e.g. onto partition using --force).

Vladimir Serbinenko <phcoder>
Group administrator
Mon 25 Apr 2011 11:44:08 AM UTC, comment #16: 


> Calling grub_util_biosdisk_flush in grub-setup makes the difference between failed and successfull install


What did you mean ?
grub_util_biosdisk_flush() is completely "read-only" i.e. does not change anything, return code does not checked anywhere.

Коренберг Марк <socketpair>
Mon 25 Apr 2011 11:08:47 AM UTC, comment #15: 


> grub_util_biosdisk_flush() tries to open device as RDONLY and flush() after that. This will not work.

Calling grub_util_biosdisk_flush in grub-setup makes the difference between failed and successfull install

Vladimir Serbinenko <phcoder>
Group administrator
Mon 25 Apr 2011 09:31:13 AM UTC, comment #14: 

I have found some errors in code and provide a patch:

grub_util_biosdisk_flush():
grub_util_biosdisk_flush() tries to open device as RDONLY and flush() after that. This will not work. As I think, it should not try to open closed device, and second, it should not try to flush devices opened read-only. Also it should check return values of fsync() and ioctl()

open_device():
In case of error during open, disk structure will be corrupted (data->dev points to free()d location. data->fd close()d, but still > 0.)
(The same issue in second place of that function)

grub_util_biosdisk_close():
simplifiction for use new grub_util_biosdisk_flush().



(file #23298)

Коренберг Марк <socketpair>
Fri 22 Apr 2011 06:01:26 PM UTC, comment #13: 

Okay, just what I want to see :) Thanks. Really.

But I have some notes about mingw.

According to http://msdn.microsoft.com/en-us/library/17618685 fsync may be implemented as:
int fsync(int fd)
{
   return _commit(fd);
}
/* Please do not implement as '#define fsync(x) _commit(x)' or '#define fsync _commit'. */

Googling says, that there is implmentation via FlushFileBuffers(), but http://support.microsoft.com/kb/99456 says, that
_commit is equivalent of FlushFileBuffers, so _commit() is preffered, as it automatically set errno in case of error.

2. Unused sync() stub for mingw should be removed, as I think.

Коренберг Марк <socketpair>
Fri 22 Apr 2011 01:15:05 PM UTC, comment #12: 

I went ahead and fixed this before rc2 since it created a real problem

Vladimir Serbinenko <phcoder>
Group administrator
Fri 15 Apr 2011 07:11:17 AM UTC, comment #11: 

Remaining part is postponed after 1.99

Vladimir Serbinenko <phcoder>
Group administrator
Fri 15 Apr 2011 04:56:38 AM UTC, comment #10: 

bump. Please reopen bug.

Коренберг Марк <socketpair>
Sun 10 Apr 2011 04:27:31 PM UTC, comment #9: 

open_device():
If we use fsync() before close(),

O_SYNC, O_FSYNC should not be used, as it give performance penalty.

When O_SYNC used, every write is blocked  until it goes to disk, and fsync() does nothing.

When O_SYNC is not used, every write is not blocked, but all modified blocks are written inside fsync(). During fsync(), blocks may be merged, so not using O_SYNC may significantly improve performance.

So, see attached patch


(file #23191)

Коренберг Марк <socketpair>
Sun 10 Apr 2011 04:14:04 PM UTC, comment #8: 

Also, i'm not
"Mark Korenberger"  :)
i'm
"Mark Korenberg"

Коренберг Марк <socketpair>
Sun 10 Apr 2011 03:41:10 PM UTC, comment #7: 


> Sync is still needed

No, it's not true.

sync() will sync all data on all (including innocent) filesystems.

So, if someone writes to another disk (suppose dropbox, email server, other disk-heavy apps), sync() never complete, and grub hang (for the time)!

fsync() is always sufficient, even without BLKFLSBUF, but BLKFLSBUF guarantee that data is flushed from disk hardware buffers to disk.

Never use sync() in any program. Also, sync() force kernel to allocate ALL blocks marked for delayed allocation....

Коренберг Марк <socketpair>
Sun 10 Apr 2011 02:08:16 PM UTC, comment #6: 

Thanks for reporting (2), fixed. Sync is still needed though because we do FS writes as well (e.g. creating/modifying core.img, copying modules)

Vladimir Serbinenko <phcoder>
Group administrator
Sat 09 Apr 2011 02:31:02 PM UTC, comment #5: 


> If user wants to install to block device (normal usage) we should call ioctl()

I mean:
"If user wants to install to block device (normal usage) we should call fsync() + ioctl()"

Also, it will be nice if errors (return values of thess functions) will be checked...

Коренберг Марк <socketpair>
Sat 09 Apr 2011 02:28:02 PM UTC, comment #4: 

Okay, you just replace close() with fsync() + ioctl.

1. The original request was to replace sync() with appropriate call(s) (see util/grub-setup.c)

2. ioctl is appropriate only if opened file was "block device". issuing BLKFLSBUF on generic file may lead to unexpected behaviour. So, if user wants to install grub into generic file (disk image), we should call fsync() only. If user wants to install to block device (normal usage) we should call ioctl()

So, please reopen bug. bug is not closed. You accidentally closed another bug.

Коренберг Марк <socketpair>
Sat 09 Apr 2011 02:14:17 PM UTC, comment #3: 

It's in bzr. r3178

Vladimir Serbinenko <phcoder>
Group administrator
Sat 09 Apr 2011 02:03:19 PM UTC, comment #2: 

Did not found changes in bzr.... how I can look to your changes?

Коренберг Марк <socketpair>
Fri 08 Apr 2011 12:01:22 PM UTC, comment #1: 

fixed.

Vladimir Serbinenko <phcoder>
Group administrator
Mon 14 Feb 2011 09:31:27 AM UTC, original submission:  

It is safer to call fsync() on FD connected to device [flush kernel cache]. After which, ioctl(BLKFLSBUF) should follow [flush hardware cache]. This sequence guaranteed data flushing.

Now, GRUB only call sync() which does nothing.

In Linux, sync() has influence only to data written to file on a filesystem. Embedding GRUB into first sectors is not such case, so sync() will not work as expected, redundant, and also have performance issues (this call will blocked until all current and ongoing data on another drive will be written, so installing GRUB on massively used server may take much time)

Коренберг Марк <socketpair>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #23298:  z.patch added by socketpair (3KiB - text/x-patch)
file #23191:  z.patch added by socketpair (480B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by phcoder (Posted a comment)
  • -email is unavailable- added by socketpair (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-04-25 socketpair Attached File- Added z.patch, #23298
    2011-04-22 phcoder StatusNone Fixed
        Open/ClosedOpen Closed
    2011-04-15 phcoder StatusFixed None
        Open/ClosedClosed Open
        SummaryGRUB2 does not flush data correctly after installing Shouldn't use sync ()
    2011-04-10 socketpair Attached File- Added z.patch, #23191
    2011-04-08 phcoder StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code