On Thu, Apr 05, 2007 at 04:18:09PM +1000, David Chinner wrote: [lots of interesting stuff] > Ah, found it. page_cache_write_begin() returns zero on success. So this: > > status = pagecache_write_begin(NULL, mapping, pos, bytes, > AOP_FLAG_UNINTERRUPTIBLE, > &page, &fsdata); > if (!status) > break; > > Will abort on success, never zero the range of the page it's supposed > to and leave without calling write_end. Then when we extend the file again.... Ah, and I just downloaded xfs-cmds :P Thanks for finding that. Sorry to waste your time with a simple thinko :( > Hmmm - an interesting question just came up. The return from ->write_end() > will either be equal to 'copied' or zero. What do you do if it returns > zero? it indicates a failure of some kind, but what? zero means that it couldn't copy anything, but there was no permanent failure (ie. we could retry as we might with any other short write). The reason why write_end returns "copied" is that some filesystems (jffs was one) that want to return a short copy. I would prefer if they could all to the required setup in write_begin so a short write couldn't occur at write_end-time, however I'm not an expert on filesystems, so I left this feature in. It will probably be easier to rip it out later than to introduce it later. I think? > In xfs_iozero, because we loop based on the number of bytes we get > returned from pagecache_write_end(); a return of zero bytes will > result in trying the same operation again. If it fails repeatedly in > the same manner (likely), then we've got an endless loop. I can > break out of the loop, but I've got no idea what the error was or > how to handle it. Any thoughts on this? Basically you can probably take some advantage of knowing that your filesystem never returns 0 here, but we should note that the filesystem must either return an error or copy some bytes eventually, rather than always return 0. I'll at that to the docs. > Also, when pagecache_write_begin() (and ->write_begin()) returns > zero, it indicates success. When pagecache_write_end() (and ->write_end()) > returns zero, it indicates some kind of failure occurred. Can > you make the return values more consistent, Nick? As I said, zero is not technically an error (and it isn't a great deal more code because most callers have to deal with short copies anyway). If we get agreement that the feature of allowing the filesystem to shorten the copy at write_end-time is unnecessary, then I would like that because it does make interfaces easier. > > Patch below Thanks a lot, applied. > > Cheers, > > Dave. > -- > Dave Chinner > Principal Engineer > SGI Australian Software Group > > --- > fs/xfs/linux-2.6/xfs_lrw.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_lrw.c > =================================================================== > --- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_lrw.c 2007-04-05 15:07:54.000000000 +1000 > +++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_lrw.c 2007-04-05 15:41:44.912532457 +1000 > @@ -151,18 +151,17 @@ xfs_iozero( > status = pagecache_write_begin(NULL, mapping, pos, bytes, > AOP_FLAG_UNINTERRUPTIBLE, > &page, &fsdata); > - if (!status) > + if (status) > break; > > memclear_highpage_flush(page, offset, bytes); > > status = pagecache_write_end(NULL, mapping, pos, bytes, bytes, > page, fsdata); > - if (status < 0) > - break; > - bytes = status; > - pos += bytes; > - count -= bytes; > + WARN_ON(status <= 0); /* can't return less than zero! */ > + pos += status; > + count -= status; > + status = 0; > } while (count); > > return (-status); - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
