Re: fs requirements for loop device?

Re: fs requirements for loop device?

[Anton Altaparmakov]

At 15:24 13/05/02, Urban Widmark wrote:
>Are there any special requirements on a filesystem in order to support the
>loop device? losetup on smbfs doesn't work and you can quickly corrupt the
>loopback file if you try it.
>
>I'm not sure what is wrong so I thought I'd ask here if there is something
>special about it (also if there is anything special vs a networked fs,
>loop.c has comments like "NFsckingS" :).
>
>If it's supposed to just work then it's probably some smbfs locking issue.
>Given that it is so easy to reproduce that should be solvable for me.
>
>If it's not, then some pointers on how to disable it so that the initial
>losetup fails before there is any corruption.

I have been told that loop device requires the hosting fs to support 
->readpage in address space operations as it uses that directly 
(apparently, I never looked at it).

Apparently, this is why one cannot create loop devices from files sitting 
on an ntfs partition when the partition is mounted with the old driver. 
Don't know about the new driver but if loop only needs ->readpage it should 
work...

Don't know if this possible information snippet is of any use but I thought 
I would throw it in...

Best regards,

         Anton


-- 
   "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: fs requirements for loop device?

[Steve Lord]

On Mon, 2002-05-13 at 09:36, Anton Altaparmakov wrote:

> 
> I have been told that loop device requires the hosting fs to support 
> ->readpage in address space operations as it uses that directly 
> (apparently, I never looked at it).
> 
> Apparently, this is why one cannot create loop devices from files sitting 
> on an ntfs partition when the partition is mounted with the old driver. 
> Don't know about the new driver but if loop only needs ->readpage it should 
> work...
> 
> Don't know if this possible information snippet is of any use but I thought 
> I would throw it in...
> 
> Best regards,
> 
>          Anton
> 

You need prepare_write and commit_write too.

Steve

Re: fs requirements for loop device?

[Christoph Hellwig]

On Mon, May 13, 2002 at 09:43:05AM -0500, Steve Lord wrote:
> > ->readpage in address space operations as it uses that directly 
> > (apparently, I never looked at it).
> 
> You need prepare_write and commit_write too.

And even worse you need locking rules similar to the generic file
operations in mm/filemap.c, this was for example biteing GFS/OpenGFS.

In short: loop.c is a horrible mess nad usually only works for
fileystems using generic_file_read/generic_file_write, an exceptions
is XFS that has very similar operations to those, but due to lack of
takeing a lock I'm still not sure whether is really is 100% correct.

An yes, I think the abuse of do_generic_file_read is a big mistake,
it should never have been exported from filemap.c.

	Christoph

Re: fs requirements for loop device?

[Andrew Morton]

Christoph Hellwig wrote:
> 
> On Mon, May 13, 2002 at 09:43:05AM -0500, Steve Lord wrote:
> > > ->readpage in address space operations as it uses that directly
> > > (apparently, I never looked at it).
> >
> > You need prepare_write and commit_write too.
> 
> And even worse you need locking rules similar to the generic file
> operations in mm/filemap.c, this was for example biteing GFS/OpenGFS.
> 
> In short: loop.c is a horrible mess nad usually only works for
> fileystems using generic_file_read/generic_file_write, an exceptions
> is XFS that has very similar operations to those, but due to lack of
> takeing a lock I'm still not sure whether is really is 100% correct.
> 
> An yes, I think the abuse of do_generic_file_read is a big mistake,
> it should never have been exported from filemap.c.
> 

Can anyone point at a reason why loop shouldn't use the
->read() and ->write() methods of the backing file's
file_operations?

AFAICS, that fixes everything, including tmpfs.

-

Re: fs requirements for loop device?

[Urban Widmark]

On Mon, 13 May 2002, Andrew Morton wrote:

> Can anyone point at a reason why loop shouldn't use the
> ->read() and ->write() methods of the backing file's
> file_operations?

The patch below is my naive implementation of that for 2.4.18.

I used to be able to reliably trigger the problem with this:
(/mnt/auto/smb is a smbfs mount to a localhost samba server exporting the
 /opt/src/smbfs/share)

# dd if=/dev/zero of=/opt/src/smbfs/share/iozone.tmp bs=1024 count=200000
...
# losetup /dev/loop0 /mnt/auto/smb/iozone.tmp
# mke2fs /dev/loop0
...
# mount /dev/loop0 /mnt/tmp
# cp -a ~puw/src/linux/linux-2.4.18/* /mnt/tmp
<something>: Input/Output error
# dmesg
EXT2-fs error (device loop(7,0)): read_block_bitmap: Cannot read block 
bitmap - block_group = 0, block_bitmap = 3
EXT2-fs error (device loop(7,0)): read_block_bitmap: Cannot read block 
bitmap - block_group = 21, block_bitmap = 172033

So it didn't require that much. But I no longer can, with the above plus
other things.

Btw, thanks everyone for the suggestions. They answer the question I
orignially had. I can't spot where smbfs is getting it wrong though. The
"aops" looks a lot like the nfs ones ... and nfs works(?). Oh well, some
other night.

/Urban


--- linux-2.4.18-orig/drivers/block/loop.c	Tue May 14 00:26:40 2002
+++ linux-2.4.18/drivers/block/loop.c	Tue May 14 00:50:49 2002
@@ -172,116 +172,44 @@
 		   loff_t pos)
 {
 	struct file *file = lo->lo_backing_file; /* kudos to NFsckingS */
-	struct address_space *mapping = file->f_dentry->d_inode->i_mapping;
-	struct address_space_operations *aops = mapping->a_ops;
-	struct page *page;
-	char *kaddr, *data;
+	mm_segment_t fs;
+	char *data;
 	unsigned long index;
-	unsigned size, offset;
-	int len;
+	unsigned offset;
+	int len, ret;
 
-	down(&mapping->host->i_sem);
 	index = pos >> PAGE_CACHE_SHIFT;
 	offset = pos & (PAGE_CACHE_SIZE - 1);
 	len = bh->b_size;
 	data = bh->b_data;
-	while (len > 0) {
-		int IV = index * (PAGE_CACHE_SIZE/bsize) + offset/bsize;
-		int transfer_result;
-
-		size = PAGE_CACHE_SIZE - offset;
-		if (size > len)
-			size = len;
-
-		page = grab_cache_page(mapping, index);
-		if (!page)
-			goto fail;
-		if (aops->prepare_write(file, page, offset, offset+size))
-			goto unlock;
-		kaddr = page_address(page);
-		flush_dcache_page(page);
-		transfer_result = lo_do_transfer(lo, WRITE, kaddr + offset, data, size, IV);
-		if (transfer_result) {
-			/*
-			 * The transfer failed, but we still write the data to
-			 * keep prepare/commit calls balanced.
-			 */
-			printk(KERN_ERR "loop: transfer error block %ld\n", index);
-			memset(kaddr + offset, 0, size);
-		}
-		if (aops->commit_write(file, page, offset, offset+size))
-			goto unlock;
-		if (transfer_result)
-			goto unlock;
-		data += size;
-		len -= size;
-		offset = 0;
-		index++;
-		pos += size;
-		UnlockPage(page);
-		page_cache_release(page);
-	}
-	up(&mapping->host->i_sem);
-	return 0;
-
-unlock:
-	UnlockPage(page);
-	page_cache_release(page);
-fail:
-	up(&mapping->host->i_sem);
-	return -1;
-}
 
-struct lo_read_data {
-	struct loop_device *lo;
-	char *data;
-	int bsize;
-};
+	fs = get_fs();
+	set_fs(get_ds());
+	ret = file->f_op->write(file, data, len, &pos);
+	set_fs(fs);
 
-static int lo_read_actor(read_descriptor_t * desc, struct page *page, unsigned long offset, unsigned long size)
-{
-	char *kaddr;
-	unsigned long count = desc->count;
-	struct lo_read_data *p = (struct lo_read_data*)desc->buf;
-	struct loop_device *lo = p->lo;
-	int IV = page->index * (PAGE_CACHE_SIZE/p->bsize) + offset/p->bsize;
-
-	if (size > count)
-		size = count;
-
-	kaddr = kmap(page);
-	if (lo_do_transfer(lo, READ, kaddr + offset, p->data, size, IV)) {
-		size = 0;
-		printk(KERN_ERR "loop: transfer error block %ld\n",page->index);
-		desc->error = -EINVAL;
-	}
-	kunmap(page);
-	
-	desc->count = count - size;
-	desc->written += size;
-	p->data += size;
-	return size;
+	return ret < 0 ? ret : 0;
 }
 
 static int lo_receive(struct loop_device *lo, struct buffer_head *bh, int bsize,
 		      loff_t pos)
 {
-	struct lo_read_data cookie;
-	read_descriptor_t desc;
 	struct file *file;
+	mm_segment_t fs;
+	int ret;
 
-	cookie.lo = lo;
-	cookie.data = bh->b_data;
-	cookie.bsize = bsize;
-	desc.written = 0;
-	desc.count = bh->b_size;
-	desc.buf = (char*)&cookie;
-	desc.error = 0;
+	/* This looks strange, if someone free's lo won't they also fput
+	   lo_backing_file? */
 	spin_lock_irq(&lo->lo_lock);
 	file = lo->lo_backing_file;
 	spin_unlock_irq(&lo->lo_lock);
-	do_generic_file_read(file, &pos, &desc, lo_read_actor);
-	return desc.error;
+
+	fs = get_fs();
+	set_fs(get_ds());
+	ret = file->f_op->read(file, bh->b_data, bsize, &pos);
+	set_fs(fs);
+
+	return ret < 0 ? ret : 0;
 }
 
 static inline int loop_get_bs(struct loop_device *lo)

Re: fs requirements for loop device?

[Andrew Morton]

Urban Widmark wrote:
> 
> On Mon, 13 May 2002, Andrew Morton wrote:
> 
> > Can anyone point at a reason why loop shouldn't use the
> > ->read() and ->write() methods of the backing file's
> > file_operations?
> 
> The patch below is my naive implementation of that for 2.4.18.
> 
> ...

 loop.c |  113 +++++++++++------------------------------------------------------
 1 files changed, 20 insertions, 93 deletions

that's a good sign ;)

> ...
> +       fs = get_fs();
> +       set_fs(get_ds());
> +       ret = file->f_op->write(file, data, len, &pos);
> +       set_fs(fs);

f_op->write() returns the number of bytes written, or a negative
errno.   So you'll need

	if (ret > 0)
		ret = 0;

in there.

Also it may be nicer to copy `pos' into a local when you
call ->read and ->write, just to avoid altering your callers'
locals, and potentially surprising them (it's OK at present,
so minor point).

> ...
> +       /* This looks strange, if someone free's lo won't they also fput
> +          lo_backing_file? */
>         spin_lock_irq(&lo->lo_lock);
>         file = lo->lo_backing_file;
>         spin_unlock_irq(&lo->lo_lock);

They do.  loop_clr_fd() does fput().

If this code is trying to actually do something legitimate
then clearly it is doing it wrongly.  Just delete the locking
I think.

This is a big improvement to loop.  Let's try to get
this into 2.4.20.


-

Re: fs requirements for loop device?

[Alexander Viro]

On Mon, 13 May 2002, Andrew Morton wrote:

> If this code is trying to actually do something legitimate
> then clearly it is doing it wrongly.  Just delete the locking
> I think.
> 
> This is a big improvement to loop.  Let's try to get
> this into 2.4.20.

Let's not.  At least try to look what does lo_read_actor() do.

Re: fs requirements for loop device?

[Andrew Morton]

Alexander Viro wrote:
> 
> On Mon, 13 May 2002, Andrew Morton wrote:
> 
> > If this code is trying to actually do something legitimate
> > then clearly it is doing it wrongly.  Just delete the locking
> > I think.
> >
> > This is a big improvement to loop.  Let's try to get
> > this into 2.4.20.
> 
> Let's not.  At least try to look what does lo_read_actor() do.

the crypto side of things can be handled by not performing
the encryption in-place, yes?

Re: fs requirements for loop device?

[Christoph Hellwig]

On Mon, May 13, 2002 at 05:13:19PM -0700, Andrew Morton wrote:
> > Let's not.  At least try to look what does lo_read_actor() do.
> 
> the crypto side of things can be handled by not performing
> the encryption in-place, yes?

This is more a 2.5 thing, but IMHO we should just kill all that
encryption/transfer stuff from the normal loop driver and instead make
it work on all filesystems.  That crypto people then can have their
own, out-of-tree copy (e.g. cloop) that still does it but only works
on "normal" filesystems.

Re: fs requirements for loop device?

[Urban Widmark]

On Mon, 13 May 2002, Andrew Morton wrote:

> f_op->write() returns the number of bytes written, or a negative
> errno.   So you'll need
> 
> 	if (ret > 0)
> 		ret = 0;

It later does 'return ret < 0 ? ret : 0;', but the diff breaks it up.
It doesn't work at all if you return the number of bytes (tried that
already :)


> This is a big improvement to loop.  Let's try to get
> this into 2.4.20.

That wasn't the point of this. If I had come back with a "I tested what
Andrew said and it was better" people would have wanted to know what I
changed.

It shows that this change makes loop-on-smbfs work. If that hints anyone
to why it doesn't work before, please let me know. Is the error in the fs
or could it be loop.c that doesn't lock everything that the read/write
interface does?


But ok ... regarding the crypto (or lo_do_transfer) what are the rules?
One block before = one block after, and size doesn't change?

The encryption would need an additional page and lo_do_transfer() would
copy to/from the real buffer.

If the extra page + extra copy is an acceptable cost for an encrypted loop
device then I think that should be possible without loop-interface
changes.

Wouldn't a separate cloop duplicate a lot of what loop.c does?


If Al thinks the fs should be fixed instead I'll try to do that, otherwise
I'll go look on lkml for someone feeling responsible for what happens to
loop.c.

/Urban

fs requirements for loop device?

[Urban Widmark]

Hello

Are there any special requirements on a filesystem in order to support the
loop device? losetup on smbfs doesn't work and you can quickly corrupt the
loopback file if you try it.

I'm not sure what is wrong so I thought I'd ask here if there is something
special about it (also if there is anything special vs a networked fs,
loop.c has comments like "NFsckingS" :).


If it's supposed to just work then it's probably some smbfs locking issue.
Given that it is so easy to reproduce that should be solvable for me.

If it's not, then some pointers on how to disable it so that the initial
losetup fails before there is any corruption.

/Urban