Bug 6227

Написал новые функции cifs_file_aio_read и cifs_file_aio_write, которые обрабатывают семантику блокировок Windows и работают с кешем согласно спецификации MS-CIFS.

Потестировал, нашёл некоторые баги - пофиксил. Пока закоммитил сюда: http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/v2.6.32-rw

Нашёл ещё баг, поправил, потестировал - перезалил новый бранч форсом.

Отправил патчи на обсуждение в upstream.

После обсуждения патчей, поправил код и снова отправил на обсуждение.

Учитывая полученные замечания, поправил код и отправил в рассылку. Посмотреть можно тут: http://news.gmane.org/gmane.linux.kernel.cifs.

Так же обновил наши ветку под 32 ядро:

http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/etersoft-rw-cache

http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/v2.6.32-rw

Исправил ошибку + сделал некоторые поправки, согласно обсуждению. Снова отправил в рассылку.

Один из патчей для non-direct режима принят в upstream:
http://git.kernel.org/?p=linux/kernel/git/sfrench/cifs-2.6.git;a=commit;h=e66673e39ac9d4749bd9676dd1caf928095409f5

У разработчиков возник вопрос, как в новой семантики strict cache будет вести себя вызов mmap_file. Занялся этим вопросом.

Выяснил, что mmap активно использует операции со страницами памяти (где сохраняется содержимое файла). Потому заставить его корректно работать с мандатори блокировками не представляется возможным без больших изменений в коде (возможно не только CIFS клиента, но и VFS). К тому же сама логика работы mmap подразумевает работу со страницами (даже файл в 1 байт будет занимать одну страницу - 4096 байт в памяти).

Потому предлагаю сделать только изменения, касающиеся 'mtime problem' (сервер может задерживать изменение времени модификации файла - тем самым мы не может ориентироваться на это значение при проверке является ли наша копия в кеше валидной или нет) - каждый раз при вызове mmap будет проверять на наличие оплока на чтение (запись+чтение) и помечать кеш невалидным в случае отсутствия оплока.

В итоге решено было добавить к предыдущему решению аналогичную проверку при msync  вызове на оплок и при его отсутствии пометить страницы памяти содержимого файла невалидными. Далее займусь реализацией.

Реализовал логику работы с mmap. Поправил write патч + добавил более подробное описание (read, write, close). Отправил в рассылку.

В процессе дальнейшего обсуждения выяснилось, что надо переписать патчи read, write и close. Если в первом надо лишь немного поправить стиль, то во втором и третьем надо сделать следующие изменения:
1) write - не сохранять в кеше информацию после записи, если у нас есть оплок на чтение, так как такая ситуация маловероятна, а в текущей реализации присутствуют трудно исправимые data racing.
2) close - решено было не обнулять весь кеш при закрытии последнего дескриптора на файл, а выставлять флаг invalid_mapping на inode.

Поправил код, согласно посту выше для 32 ядра. Выложил на git.eter. Дальше займусь накладыванием изменений на патчи для upstream.

Подготовил и отправил патчи в рассылку.

Сделал некоторые исправления в патче close.

Привёл в порядок и залил актуальные ветки rw-to-kernel и master на git.eter.

Исследовал проблему поднятую в обсуждении патча close. Нам действительно не надо дополнительно проверять флаг invalid_mapping при открытии файла, так как это делается до этого функцией cifs_d_revalidate при обращении к dentry файла. Проверял на 36 ядре - самое близкое к апстриму, что есть в рпм. Дальше буду проверять на 32 ядре.

Проверка на 32 ядре показала, что cifs_d_revalidate не всегда успевает закончить invalidate_remote_inode перед чтением из файла и приблизительно на 1 раз из 10 результат чтения ошибочный. Завтра ещё раз проверю на 36 ядре, и, в случае подтверждения различного поведения, посмотрю, чем отличается в этом контексте код 36 ядра от 32.

При более глубоком рассмотрении оказалось, что на 36 ядре такая же проблема - клиент порой (пару раз на 20 прогонов) не обнуляет кеш при invalidate_remote_inode (не успевает, обламывается, ...?). Поднял вопрос в рассылке. Посмотрим, как там прокомментируют это.

Сама функция invalidate_remote_inode не обнуляет кеш у страниц памяти, которые помечены как dirty, залочены, или находятся в таблице страниц.

Попробовал использовать функцию invalidate_inode_pages2, которая обнуляет все страницы, и проблема исчезла. Написал второе письмо в рассылку с этой информацией.

Заметил так же, что nfs клиент в ядре так же использует invalidate_inode_pages2 при обнулении страниц кеша:

 837 static int nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
 838 {
 839 >-------struct nfs_inode *nfsi = NFS_I(inode);
 840 >-------
 841 >-------if (mapping->nrpages != 0) {
 842 >------->-------int ret = invalidate_inode_pages2(mapping);
 843 >------->-------if (ret < 0)
 844 >------->------->-------return ret;
 845 >-------}

Так же написал об этом в рассылку.

Закоммитил изменения в etersoft-rw-cache и v2.6.32-rw и залил на git.eter. Создал ветку invalidate-patch - изменения в логике invalidate_mapping, основанные на ветке ядра.

Пытался найти в коде CIFS, где мы можем захватить страницу памяти, что её пропускает invalidate_remote_inode. Никаких подозрительныхт мест не нашёл. Возможно это какой-нибудь процесс VFS. В любом случае это не deadlock, так как invalidate_inode_pages2 не зависает (она используется lock_page для каждой страницы).

Переотправил патч close. Привёл в актуальное состояние ветки на git.eter.

В связи с возникшей путаницей в патчах, меня попросили переотправить всю группу. Разобрал патчи, выставил версии и отправил в рассылку. Так написал письмо для разяъяснения ситуации с процессом продвижения патчей - так как пока он мне не совсем ясен (касательно версий патчей, порядка отправки и прочего).

Встал вопрос, как соотносится данная семантика с fscache - возможностью кеширования данных файловой системой на локальный диск. Ознакомился с данным функционалом и пришёл к выводу, что эти опции strictcache и fscache должны быть взаимоисключаемыми. Написал об этом в рассылку.

Так же возник вопрос о том, что это будет плохо работать с short read - чтением, которое возвращает меньше байт, чем запрашивается. Пока не нашёл в чём так проблема и попросил прояснить ситуацию подробнее.

Выяснил в чём была проблема с векторным чтением. Отписался об этом в рассылку, чтобы люди зря не отвечали.

Проблема заключается в неправильном понимании логики работы aio_read/write вызовов. Буду смотреть, как это можно поправить.

Отправил патчи read, write, fsync, mmap заново с учётом всех замечаний (fscache, vectored i/o).

Поправил стиль комментариев в патчах read и write. Жду комментариев над текущим вариантом.

Получил комментарии по текущим патчам. Обнаружилось узкое место - когда пользователь указывает в векторном чтении/записи много маленьких запросов, мы отправляем их по очереди. Гораздо быстрее будет объединить их в несколько больших запросов. Далее займусь этим.

Сделал анализ существующих возможностей модуля для реализации вышеописанной идеи. Набросал пробный код. Далее займусь реализацией функций cifs_file_aio_read и cifs_file_aio_write.

Попробовал пробную реализацию в действии и обнаружил, что данная идея не может быть применена для strict cache режима из-за возможного наличия жёстких блокировок на одном из участков, что сломает всю запись (чтение), как будто бы блокировки присутствуют на всех участвахт вызова writev(readv). Написал об этом в рассылку. Жду комментариев.

Как мне разъяснили в рассылке, нам следует расценивать векторное чтение (запись) как обычную запись (чтение) только с участием нескольких буфферов для данных. Поэтому мы можем объединять маленькие запросы в большие для экономии траффика.

amorozov@, используется ли в wine операции readv и writev?

> amorozov@, используется ли в wine операции readv и writev?

Для работы с обычными файлами не используются, writev используется для записи в пайп при взаимодействии запущенных в wine программ и wineserver.

(В ответ на comment #35)
> > amorozov@, используется ли в wine операции readv и writev?
> 
> Для работы с обычными файлами не используются, writev используется для записи в
> пайп при взаимодействии запущенных в wine программ и wineserver.

Ок, в таком случае можно реализовывать вариант объединения буфферов.

Переделал патчи, добавив туда новые структуры файловых операций для strict cache режима. Теперь они будут выбираться ещё во время монтирования, чтобы избежать дополнительных проверок в дальнейшем. Согласно этому обновил ветку rw-to-kernel на git.eter. Дальше попробую новую реализацию на тестах.

Потестировал, обнаржил ошибку и некоторые неточности - исправил. Перезалил форсом ветку на git.eter, подготовил и отправил патчи в рассылку.

Получил ответ. Рекомендуют избежать дополнительных копирований из памяти в память. Продумал и обсудил как этого избежать. Далее приступлю к реализации.

Написал реализация для операции чтения. Немного потестировал. Далее продолжу тестирование, потом приступлю к реализации операции записи.

Обсудил текущую ситуацию по патчам. На данный момент надо сделать следующее:
1) закончить чтение.
2) запись оставить такой же, только добавить комментарии для последующей реализации отправки напрямую в случае отсутствия подписи пакетов в сессии.
3) объединить патчи 2 и 3, заменив нереализованные новые функции старыми.

Сделал описанное в предыдущем посте. Оформил новые версии патчей и отправил в рассылку.

В ходе обсуждения обнаружились ошибка в данной модификации write патча, а так же некоторые недочёты в read и mmap патчах.

На данный момент следующие патчи согласованны:
1) close;
2) fsync;
6) mount option.

Поправил mmap и исправил read патчи.

Исправил сломавшуюся ветку (предположительно из-за коммитов в бранч без имени), проверил и отправил патчи read и mmap.  Начал работу над патчем write.

Набросал новый вариант функции cifs_strict_write с использованием отдельных страниц памяти. Дальше буду дописывать его и пробовать в работе.

Ознакомиться можно тут:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=commitdiff;h=04ef8111d4371549260496e3aaa0e68ca64645de

Написал работающий код cifs_strict_write:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=commitdiff;h=ea32baaa69273c9649c487cb17d2e12bcd8da1a2

Протестировал вышеописанный код, поправил, провёл некоторый рефакторинг. Дальше буду дорабатывать стиль, чтобы можно было оформить в патч.

Удалил ветку rw-to-kernel2, закоммитил последние изменения по cifs_strict_write в rw-to-kernel. Отправил патч в рассылку linux-cifs.

На данный момент согласованны следующие патчи:
1) close;
2) fsync;
3) mmap;
6) mount option.

Осталось разобраться с read и write.

Изучил комментарии по патчам read и write и провёл изучение стандартных средств для работы с векторами массивов. Как выяснилось, много из того, что я написал руками, можно заменить стандартными функциями ядра, что является более правильным. Собираюсь переписать патчи с использованием этих функций.

Переписал патчи read и write. После более тщательного тестирования отправлю в рассылку.

Обнаружил ошибки в write патче - исправил. Добился правильной работы на тестах чтения/записи. Возник вопрос, связанный с реализацией копирования копирования из вектора буфферов в страницу - отправил вопрос Jeff Layton'у. Пока оставил вариант, который применяется в ядре, но у меня вроде другая ситуация - страницы памяти локальные и другие потоки их не трогают.

Ознакомился с системой обработки pagefault в ядре. Поправил write патч - убрал atomic операцию копирования и pagefault_disable() и pagefaul_enable() вызовы до и после.

Провёл стресс тестирование (чтение/запись/чтение-проверка) ~5 гигабайтового файла. Ошибок не обнаружено. Отправил read и write патчи в рассылку.

Провёл небольшое исследование на тему переполнения в readv с использованием memcpy_toiovecend функции. Было сомнение, что данная функция переполняет int в readv вызове, но они не подтвердились - общая длина вектора ограничивается MAX_RW_COUNT функцией rw_copy_check_uvector в VFS, а 
MAX_RW_COUNT в свою очередь приводится к int.

read патч: http://article.gmane.org/gmane.linux.kernel.cifs/2160
write патч: http://article.gmane.org/gmane.linux.kernel.cifs/2161

Получил комментарии по патчам read и write:
1) read - всё ок, только надо сделать один глобальный вызов статичным (я планировал использовать его в дальнейшем, но лучше сделать его глобальным, именно когда он понадобится).
2) write - в цикле while (rc == -EAGAIN) {...} надо проверять на переустановку соединения.

Отправил патчи:
1) read - убрал глобльное определение, убрал проверку на NULL private data файла. http://article.gmane.org/gmane.linux.kernel.cifs/2191
2) write - убрал глобальное определение, добавил проверку на переустановку соединения и проверку на возможность записи. http://article.gmane.org/gmane.linux.kernel.cifs/2192

Все патчи получили reviewed-by тэг от Джефа Лэйтона. Отправил их Стиву Френчу для соединения с основной веткой.

Собрал strictcache под 37 ядро в etercifs-4.7.0-alt1 на ftp.

Нужно интегрировать в патч https://patchwork.kernel.org/patch/433271/ в ветку 37 ядра со strictcache и собрать новую сборку для тестирования (etercifs-4.7.1).

Интегрировал вышеописанный патч в исходники 37 ядра сборки 4.7.0. Собрал etercifs-4.7.1-alt1 на ftp.

Патч, исправляющий логику работы с оплоками уже в апстриме: http://git.kernel.org/?p=linux/kernel/git/sfrench/cifs-2.6.git;a=commitdiff;h=b9d53658f4c6b6a914baf5b47046fb2e1885d7bd;hp=a25cecce88194b2edf38b8c3b1665e9318eb2d22

Теперь надо собрать новые сборки etercifs-4.7.3 (без патча writepages) и 4.7.4 (с патчем writepages).

Собрал etercifs-4.7.2 (без writepages патчем) и etercifs-4.7.3 (с writepages патчем) - в предыдущем посте ошибся с нумерацией.

Портировал strict cache режим на ядро v2.6.35. http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/etersoft-strict-cache-2.6.35
Далее планирую собрать ветку etercifs и включить его в тестовые сборки.

strict cache режим в ветке апстрим. Обновил master на git.eter.

Поправил типы переменных в write и read патчах, по сообщению в рассылке о предупреждениях компилятора. Прокинул в соответствующие ветки и залил на git.eter.

Выяснилось, что предыдущий патч плохо накладывается на уже отправленный но не смерженный патч. Исправил и отправил снова.

Последний патч, исправляющий типы переменных, приняли в апстрим. Обновил master на git.eter.

Текущая бага решена. Дальнейшие работы внедрению кода в etercifs будут идти в отдельных багах, о которых будет писаться тут: http://bugs.etersoft.ru/show_bug.cgi?id=6765.

Решена.