debugfs: dump a sparse file as a new sparse file

lib/ext2fs/fileio.c errcode_t ext2fs_file_close(ext2_file_t file) · line 245

Quoted 3 lines · Zheng Liu

lib/ext2fs/fileio.c +3 -0

+errcode_t ext2fs_file_read2(ext2_file_t file, void *buf,
+			    unsigned int wanted, unsigned int *got,
+			    ext2_off64_t *seek)

I'm a bit concenred about this abstraction. Consider what happens if wanted is greater than a block size --- for example, consider if wanted is 16k, and every other 1k block is uninitialized.

Then ext2fs_file_read2() will return *got set to 8k, and *seek set to 8k, and the buffer will contain the blocks that are initialized packed up right against each other.

Worse, ext2fs_file_read() will do the same thing, so this commit changes how ext2fs_file_read() functions, and a program which expects to get the correct contents from the file will malfunction.

Quoted 4 lines

I'm a bit concenred about this abstraction. Consider what happens if wanted is greater than a block size --- for example, consider if wanted is 16k, and every other 1k block is uninitialized.

Hi Ted,

I wonder why wanted is 16k. If a program calls ext2fs_file_read() function, seek will be 0 and SEEK flag won't be marked. The behavior of ext2fs_file_read() is the same as before. If ext2fs_file_read2() is called by dump_file(), seek won't be 0 and wanted is always equal to block size. That is why I fix the hard-coded buffer length in dump_file(). If I miss something, please let me know.

Quoted 5 lines · Zheng Liu

Quoted 4 lines

I'm a bit concenred about this abstraction. Consider what happens if wanted is greater than a block size --- for example, consider if wanted is 16k, and every other 1k block is uninitialized.

Quoted 8 lines

Hi Ted,

I wonder why wanted is 16k. If a program calls ext2fs_file_read() function, seek will be 0 and SEEK flag won't be marked. The behavior of ext2fs_file_read() is the same as before. If ext2fs_file_read2() is called by dump_file(), seek won't be 0 and wanted is always equal to block size. That is why I fix the hard-coded buffer length in dump_file(). If I miss something, please let me know.

The problem is that ext2fs_file_read() is an exported function, and there are users of this API/ABI outside of e2fsprogs.

The goal of this function is that it should look like the read system call, and the caller might not know what the blocksize might be. So if the caller uses a 4k fixed size buffer, and the underlying file system blocksize is 1k, this function needs to work properly.

So consider what happens if some program, perhaps an ext[234] FUSE driver (there are two or three of them out there), or the e2tools package, uses a 4k or 16k buffer --- this is legal, and they call the existing ext2fs_file_read() library function. In your patch, ext2fs_file_read() will call ext2fs_file_read2(), and it will skip the sparse blocks, and since the returned seek pointer is null, there's no possible way for the caller of the ext2fs_file_read() would know this had happened --- and even if there was a way, we don't ever change the semantics/behaviour of an existing functional interface unless it's a clear bug (and even then we need to think very carefully about the backwards compatibility implications).

Yes, some programs call ext2fs_file_read() with a 4k or 16k fixed size buffer, and ext2fs_file_read() calls ext2fs_file_read2(). But it won't skip the sparse blocks because when ext2fs_file_read2() is called in ext2fs_file_read(), the last argument, namely 'seek', is 0. That means that in ext2fs_file_read2() 'flags' is 0. Thus, in load_buffer() 'flags' is not equal to SEEK, and EXT2_FILE_BUF_VALID is marked. Then we return back to ext2fs_file_read2() and all data in file->buf is copied. So I think the behavior of ext2fs_file_read() doesn't be changed.

Quoted 9 lines · Zheng Liu

Yes, some programs call ext2fs_file_read() with a 4k or 16k fixed size buffer, and ext2fs_file_read() calls ext2fs_file_read2(). But it won't skip the sparse blocks because when ext2fs_file_read2() is called in ext2fs_file_read(), the last argument, namely 'seek', is 0. That means that in ext2fs_file_read2() 'flags' is 0. Thus, in load_buffer() 'flags' is not equal to SEEK, and EXT2_FILE_BUF_VALID is marked. Then we return back to ext2fs_file_read2() and all data in file->buf is copied. So I think the behavior of ext2fs_file_read() doesn't be changed.

You're right; I had forgotten about that part of the change.

I still am a bit concerned about the interface, because if you specify a pointer to seek in ext2fs_file_read2(), you have to know what the file system blocksize is, because if you give a count which is larger than a single block, the value of the returned seek and the data which is returned in the buffer is impossible to interpret (consider a file where every other 1k block is sparse, and you try to read into a 4k buffer).

So what I would suggest is the following as a better, more efficient interface.

1) Add a new flag which can be passed into ext2_file_open() which requests sparse-intelligent handling.

2) If the sparse flag is set, then ext2_file_read() will stop the read when it runs into the first uninitialized or sparse block. That is, consider the example file which has 8k of data, a 4k uninitialized block, and then 12k of data after that. If the sparse flag is passed to ext2_file_open(), then ext2fs_file_read(fd, buf, 16384, &got) will read 8k of data into buf, and return with got set to 8192.

3) To distinguish between EOF and a sparse block, if the current file offset is pointed at a sparse/uninitialized block, and the sparse flag was passed to ext2_file_open(), then in addition to *got getting set 0, ext2_file_read() will also return a new error code, EXT2_ET_READ_HOLE_FOUND.

4) We also extend ext2_file_llseek() to also support EXT2_SEEK_HOLE and EXT2_SEEK_DATA, which works like SEEK_HOLE and SEEK_DATA flags to llseek(). This will allow the caller to efficiently find the next part of the file with valid data.

What I like about this interface is that we don't need to define a new ext2_file_read2(), and it is also more efficient for an application which is interested in reading multiple blocks at a time.

What do you think?

2) If the sparse flag is set, then ext2_file_read() will stop the read when it runs into the first uninitialized or sparse block. That is, consider the example file which has 8k of data, a 4k uninitialized block, and then 12k of data after that. If the sparse flag is passed to ext2_file_open(), then ext2fs_file_read(fd, buf, 16384, &got) will read 8k of data into buf, and return with got set to 8192.

3) To distinguish between EOF and a sparse block, if the current file offset is pointed at a sparse/uninitialized block, and the sparse flag was passed to ext2_file_open(), then in addition to *got getting set 0, ext2_file_read() will also return a new error code, EXT2_ET_READ_HOLE_FOUND.