Skip to content

Commit

Permalink
loop: remove the incorrect write_begin/write_end shortcut
Browse files Browse the repository at this point in the history 
Currently the loop device tries to call directly into write_begin/write_end
instead of going through ->write if it can.  This is a fairly nasty shortcut
as write_begin and write_end are only callbacks for the generic write code
and expect to be called with filesystem specific locks held.

This code currently causes various issues for clustered filesystems as it
doesn't take the required cluster locks, and it also causes issues for XFS
as it doesn't properly lock against the swapext ioctl as called by the
defragmentation tools.  This in case causes data corruption if
defragmentation hits a busy loop device in the wrong time window, as
reported by RH QA.

The reason why we have this shortcut is that it saves a data copy when
doing a transformation on the loop device, which is the technical term
for using cryptoloop (or an XOR transformation).  Given that cryptoloop
has been deprecated in favour of dm-crypt my opinion is that we should
simply drop this shortcut instead of finding complicated ways to to
introduce a formal interface for this shortcut.

Signed-off-by: Christoph Hellwig <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
  • Loading branch information
@axboe
Christoph Hellwig authored and axboe committed Oct 17, 2011
1 parent 8bc03e8 commit 456be14
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 113 deletions.
135 changes: 23 additions & 112 deletions drivers/block/loop.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,74 +202,6 @@ lo_do_transfer(struct loop_device *lo, int cmd,
return lo->transfer(lo, cmd, rpage, roffs, lpage, loffs, size, rblock);
}

/**
* do_lo_send_aops - helper for writing data to a loop device
*
* This is the fast version for backing filesystems which implement the address
* space operations write_begin and write_end.
*/
static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
loff_t pos, struct page *unused)
{
struct file *file = lo->lo_backing_file; /* kudos to NFsckingS */
struct address_space *mapping = file->f_mapping;
pgoff_t index;
unsigned offset, bv_offs;
int len, ret;

mutex_lock(&mapping->host->i_mutex);
index = pos >> PAGE_CACHE_SHIFT;
offset = pos & ((pgoff_t)PAGE_CACHE_SIZE - 1);
bv_offs = bvec->bv_offset;
len = bvec->bv_len;
while (len > 0) {
sector_t IV;
unsigned size, copied;
int transfer_result;
struct page *page;
void *fsdata;

IV = ((sector_t)index << (PAGE_CACHE_SHIFT - 9))+(offset >> 9);
size = PAGE_CACHE_SIZE - offset;
if (size > len)
size = len;

ret = pagecache_write_begin(file, mapping, pos, size, 0,
&page, &fsdata);
if (ret)
goto fail;

file_update_time(file);

transfer_result = lo_do_transfer(lo, WRITE, page, offset,
bvec->bv_page, bv_offs, size, IV);
copied = size;
if (unlikely(transfer_result))
copied = 0;

ret = pagecache_write_end(file, mapping, pos, size, copied,
page, fsdata);
if (ret < 0 || ret != copied)
goto fail;

if (unlikely(transfer_result))
goto fail;

bv_offs += copied;
len -= copied;
offset = 0;
index++;
pos += copied;
}
ret = 0;
out:
mutex_unlock(&mapping->host->i_mutex);
return ret;
fail:
ret = -1;
goto out;
}

/**
* __do_lo_send_write - helper for writing data to a loop device
*
Expand Down Expand Up @@ -297,10 +229,8 @@ static int __do_lo_send_write(struct file *file,
/**
* do_lo_send_direct_write - helper for writing data to a loop device
*
* This is the fast, non-transforming version for backing filesystems which do
* not implement the address space operations write_begin and write_end.
* It uses the write file operation which should be present on all writeable
* filesystems.
* This is the fast, non-transforming version that does not need double
* buffering.
*/
static int do_lo_send_direct_write(struct loop_device *lo,
struct bio_vec *bvec, loff_t pos, struct page *page)
Expand All @@ -316,15 +246,9 @@ static int do_lo_send_direct_write(struct loop_device *lo,
/**
* do_lo_send_write - helper for writing data to a loop device
*
* This is the slow, transforming version for filesystems which do not
* implement the address space operations write_begin and write_end. It
* uses the write file operation which should be present on all writeable
* filesystems.
*
* Using fops->write is slower than using aops->{prepare,commit}_write in the
* transforming case because we need to double buffer the data as we cannot do
* the transformations in place as we do not have direct access to the
* destination pages of the backing file.
* This is the slow, transforming version that needs to double buffer the
* data as it cannot do the transformations in place without having direct
* access to the destination pages of the backing file.
*/
static int do_lo_send_write(struct loop_device *lo, struct bio_vec *bvec,
loff_t pos, struct page *page)
Expand All @@ -350,17 +274,16 @@ static int lo_send(struct loop_device *lo, struct bio *bio, loff_t pos)
struct page *page = NULL;
int i, ret = 0;

do_lo_send = do_lo_send_aops;
if (!(lo->lo_flags & LO_FLAGS_USE_AOPS)) {
if (lo->transfer != transfer_none) {
page = alloc_page(GFP_NOIO | __GFP_HIGHMEM);
if (unlikely(!page))
goto fail;
kmap(page);
do_lo_send = do_lo_send_write;
} else {
do_lo_send = do_lo_send_direct_write;
if (lo->transfer != transfer_none) {
page = alloc_page(GFP_NOIO | __GFP_HIGHMEM);
if (unlikely(!page))
goto fail;
kmap(page);
do_lo_send = do_lo_send_write;
}
}

bio_for_each_segment(bvec, bio, i) {
ret = do_lo_send(lo, bvec, pos, page);
if (ret < 0)
Expand Down Expand Up @@ -849,35 +772,23 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
mapping = file->f_mapping;
inode = mapping->host;

if (!(file->f_mode & FMODE_WRITE))
lo_flags |= LO_FLAGS_READ_ONLY;

error = -EINVAL;
if (S_ISREG(inode->i_mode) || S_ISBLK(inode->i_mode)) {
const struct address_space_operations *aops = mapping->a_ops;

if (aops->write_begin)
lo_flags |= LO_FLAGS_USE_AOPS;
if (!(lo_flags & LO_FLAGS_USE_AOPS) && !file->f_op->write)
lo_flags |= LO_FLAGS_READ_ONLY;
if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
goto out_putf;

lo_blocksize = S_ISBLK(inode->i_mode) ?
inode->i_bdev->bd_block_size : PAGE_SIZE;
if (!(file->f_mode & FMODE_WRITE) || !(mode & FMODE_WRITE) ||
!file->f_op->write)
lo_flags |= LO_FLAGS_READ_ONLY;

error = 0;
} else {
goto out_putf;
}
lo_blocksize = S_ISBLK(inode->i_mode) ?
inode->i_bdev->bd_block_size : PAGE_SIZE;

error = -EFBIG;
size = get_loop_size(lo, file);

if ((loff_t)(sector_t)size != size) {
error = -EFBIG;
if ((loff_t)(sector_t)size != size)
goto out_putf;
}

if (!(mode & FMODE_WRITE))
lo_flags |= LO_FLAGS_READ_ONLY;
error = 0;

set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);

Expand Down
1 change: 0 additions & 1 deletion include/linux/loop.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ struct loop_device {
*/
enum {
LO_FLAGS_READ_ONLY = 1,
LO_FLAGS_USE_AOPS = 2,
LO_FLAGS_AUTOCLEAR = 4,
};

Expand Down

0 comments on commit 456be14

Please sign in to comment.