btrfs-progs: btrfs_close_devices(): only fsync() if device->writeable
diff mbox

Message ID CA+X5Wn6h8sot6Z7OQVvehD_T4XxHOiss=gT18s_d6EJPZrUhQA@mail.gmail.com
State New
Headers show

Commit Message

james harvey June 6, 2018, 6:30 a.m. UTC
Prevent unnecessary error from failing fsync(), if opened read only.

Performed 'grep "writeable = " *.h *.c' to make sure there were no odd
situations where fsync() might still be desired here.  They're all straight-
forward.  The only situation where writeable will be 0 is if btrfs_open_devices
is given flags without O_RDWR.  There is no situation where a writeable volume
temporarily becomes unwriteable, or anything like that.  Given that it's being
opened O_RDWR, there's no reason to attempt fsync().

utils.c

   int btrfs_add_to_fsid() {
      ...
      device->writeable = 1;

volumes.c

   int btrfs_close_devices() {
      ...
      while (!list_empty(&fs_devices->devices)) {
         ...
         // just after the fsync() being patched
267:     device->writeable = 0;
   ...
   int btrfs_open_devices() {
      ...
      list_for_each_entry(device, &fs_devices->devices, dev_list) {
         ...
         if (flags & O_RDWR)
332:        device->writeable = 1

kernel btrfs_close_devices() does not have a corresponding fsync() that I see.

Signed-off-by: James Harvey <jamespharvey20@gmail.com>
---
 volumes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.17.0
--
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

Comments

David Sterba July 18, 2018, 2:06 p.m. UTC | #1
On Wed, Jun 06, 2018 at 02:30:41AM -0400, james harvey wrote:
> Prevent unnecessary error from failing fsync(), if opened read only.
> 
> Performed 'grep "writeable = " *.h *.c' to make sure there were no odd
> situations where fsync() might still be desired here.  They're all straight-
> forward.  The only situation where writeable will be 0 is if btrfs_open_devices
> is given flags without O_RDWR.  There is no situation where a writeable volume
> temporarily becomes unwriteable, or anything like that.  Given that it's being
> opened O_RDWR, there's no reason to attempt fsync().
> 
> utils.c
> 
>    int btrfs_add_to_fsid() {
>       ...
>       device->writeable = 1;
> 
> volumes.c
> 
>    int btrfs_close_devices() {
>       ...
>       while (!list_empty(&fs_devices->devices)) {
>          ...
>          // just after the fsync() being patched
> 267:     device->writeable = 0;
>    ...
>    int btrfs_open_devices() {
>       ...
>       list_for_each_entry(device, &fs_devices->devices, dev_list) {
>          ...
>          if (flags & O_RDWR)
> 332:        device->writeable = 1
> 
> kernel btrfs_close_devices() does not have a corresponding fsync() that I see.
> 
> Signed-off-by: James Harvey <jamespharvey20@gmail.com>

Applied, thanks.
--
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

Patch
diff mbox

diff --git a/volumes.c b/volumes.c
index c6e34321..36d1bde6 100644
--- a/volumes.c
+++ b/volumes.c
@@ -254,7 +254,7 @@  again:
                device = list_entry(fs_devices->devices.next,
                                    struct btrfs_device, dev_list);
                if (device->fd != -1) {
-                       if (fsync(device->fd) == -1) {
+                       if (device->writeable && fsync(device->fd) == -1) {
                                warning("fsync on device %llu failed: %m",
                                        device->devid);
                                ret = -errno;